Additional SSH key pair Algorithms #40
Conversation
BeRT2me
requested review from
alexlatchford,
andyg-zg,
AntonKueltz,
gideonw,
jonathannaz and
rajakshay
as code owners
tests/test_core.py
Outdated
| template_oid = installed_repo.references.get('refs/heads/template').target | ||
| installed_repo.branches.remote.create('origin/template', installed_repo[template_oid]) | ||
| installed_repo.branches.local.delete('template') | ||
| # def test_upgrade_fetches_remote_template(installed_repo: Repository, template_repo: Repository): |
There was a problem hiding this comment.
Why are you commenting out this test?
There was a problem hiding this comment.
Purely a mistake on my part, this couldn't run locally, and I forgot to re-add it.
|
@BeRT2me - Can you provide more context around the other changes you made beyond adding support for the additional algorithms? The amount of changes makes it harder to review. |
| @@ -109,7 +109,7 @@ pytest | |||
| To run linting: | |||
|
|
|||
| ```bash | |||
| flake8 --config flake8.cfg battenberg | |||
| flake8 --config flake8.cfg . | |||
There was a problem hiding this comment.
Lint's tests as well as battenberg
This is where many changes likely came from.
| def test_construct_keypair_defaults(Keypair: Mock, MockPath: Mock, tmp_path: Path): | ||
| MockPath.return_value = tmp_path | ||
| public_key_path = tmp_path / 'id_rsa.pub' | ||
| public_key_path.touch() | ||
| private_key_path = tmp_path / 'id_rsa' | ||
| private_key_path.touch() | ||
| construct_keypair() | ||
| user_home = os.path.expanduser('~') | ||
| Keypair.assert_called_once_with('git', f'{user_home}/.ssh/id_rsa.pub', | ||
| f'{user_home}/.ssh/id_rsa', '') | ||
| Keypair.assert_called_with('git', public_key_path, | ||
| private_key_path, '') |
There was a problem hiding this comment.
Since construct_keypair now checks if the files exists, Mocks are used to temporarily create the expected files.
battenberg/utils.py
Outdated
| ALGORITHMS = { | ||
| "ED25519": { | ||
| "public": "id_ed25519.pub", | ||
| "private": "id_ed25519" | ||
| }, | ||
| "ED25519_SK": { | ||
| "public": "id_ed25519_sk.pub", | ||
| "private": "id_ed25519_sk" | ||
| }, | ||
| "ECDSA_SK": { | ||
| "public": "id_ecdsa_sk.pub", | ||
| "private": "id_ecdsa_sk" | ||
| }, | ||
| "RSA": { | ||
| "public": "id_rsa.pub", | ||
| "private": "id_rsa" | ||
| } | ||
| } | ||
|
|
||
|
|
||
| def find_key_paths(ssh_path: Path): | ||
| for algorithm in ALGORITHMS.values(): | ||
| public_key_path = ssh_path / algorithm['public'] | ||
| private_key_path = ssh_path / algorithm['private'] | ||
| if public_key_path.exists() and private_key_path.exists(): | ||
| return public_key_path, private_key_path | ||
|
|
||
| raise KeypairException(f'Could not find keypair in {ssh_path}', | ||
| f'Possible options include: {ALGORITHMS}') | ||
|
|
||
|
|
||
| def construct_keypair(ssh_path: Path = None, passphrase: str = '') -> Keypair: | ||
| ssh_path = ssh_path or Path('~/.ssh').expanduser() | ||
| public_key_path, private_key_path = find_key_paths(ssh_path) |
There was a problem hiding this comment.
Core Issue being solved, Gitlab accepts more than just RSA key pairs. A new exception type was added for when none of the possibilities can be located.
The original public_key_path, private_key_path arguments were only ever used in testing, so I simplified to ssh_path
| def test_open_repository(): | ||
| path = 'test-path' | ||
| with pytest.assertRaises(ValueError) as e: | ||
| open_repository(path) | ||
|
|
||
| assert str(e.value) == f'{path} is not a valid repository path.' |
There was a problem hiding this comment.
For some reason this test was written twice, I updated the other duplicate ~
There was a problem hiding this comment.
All changes are conversions to pathlib
There was a problem hiding this comment.
All changes are due to applying linting rules to tests.
There was a problem hiding this comment.
All changes are conversions to pathlib or due to applying linting rules to tests.
There was a problem hiding this comment.
Other changes are conversions to pathlib or due to applying linting rules to tests.
| def test_construct_keypair_missing_key(MockPath: Mock, tmp_path: Path): | ||
| MockPath.return_value = tmp_path | ||
| with pytest.raises(KeypairMissingException): | ||
| construct_keypair() |
There was a problem hiding this comment.
Added an additional test for when the error KeypairMissingException is raised due to missing files.
| @pytest.mark.parametrize('algorithm', ALGORITHMS.values()) | ||
| def test_construct_keypair(Keypair: Mock, tmp_path: Path, algorithm: dict[str, Path]): | ||
| public_key_path = tmp_path / algorithm['public'] | ||
| public_key_path.touch() | ||
| private_key_path = tmp_path / algorithm['private'] | ||
| private_key_path.touch() | ||
| passphrase = 'test-passphrase' | ||
| construct_keypair(public_key_path, private_key_path, passphrase) | ||
| Keypair.assert_called_once_with('git', public_key_path, private_key_path, passphrase) | ||
| construct_keypair(tmp_path, passphrase) | ||
| Keypair.assert_called_with('git', public_key_path, private_key_path, passphrase) |
|
@andyg-zg I've added some comments explaining the changes that were made - apart from adding support for the additional algorithms. The changes fall into three categories: |
I use an ED25519 key, and was receiving this error message:
Many types of keys are supported by Gitlab: https://docs.gitlab.com/ee/user/ssh.html#supported-ssh-key-types
This issue was that battenberg only looks for an RSA key!
This PR resolves that issue ~ and makes some minor improvements.