Feature/fix 20 #21

mirisbowring · 2024-07-01T15:08:20Z

Hi,

i debugged this plugin and found 2 errors that lead to #20 .

1. Relative Paths

Line 355 in 6bb3f2d

with open(identifier, "rb") as f:

Since this plugin reads a possible public key from gitconfig, this line resulted in an Error:

[Errno 2] No such file or directory: '~/.ssh/id_ed25519.pub'

To fix this issue I propose to use os.path.expanduser to fix this common mistake:

https://github.com/mirisbowring/gitbark-core/blob/d30c7ed2fcd6c0d172f4f7cd7af6433ef3553398/bark_core/signatures.py#L356

2. False starting index

During the __init__ of the SshKey you are splitting the read public key to extract the type and the key itself. But during the while iteration you start at 1 instead of 0 which results in an infinite loop until python reaches an out of index since i > len(parts) is never catched.

My proposed fix: Start with index 0

https://github.com/mirisbowring/gitbark-core/blob/d30c7ed2fcd6c0d172f4f7cd7af6433ef3553398/bark_core/signatures.py#L325

3. SSH comment interpreted as mail

While testing the bark verify i experienced a behaviour, that ssh signatures (though they are valid) are marked as invalid (without a reason). I did a deep dive into the code and discovered the following problem:

gitbark-core/bark_core/signatures.py

Line 324 in 6bb3f2d

self._emails = parts[0].decode().split(",")

The line above ready the first part of the public key as email which will NEVER work since the first part of the public key is always the keyspec like ssh_ed25519. Therefor, the mail Validation in L342 will always fail:

gitbark-core/bark_core/signatures.py

Line 342 in 6bb3f2d

if email not in self._emails:

To fix this issue, i set the index of the parts list to 2 (remember the format: keyspec_pubkey_comment):

https://github.com/mirisbowring/gitbark-core/blob/433326c1f1d256260047afa821fdafd7b731d312/bark_core/signatures.py#L324

I somehow understand why this check is implemented. Anyway: I strongly recommend to hint this behaviour in the user docs! Since this last part of the public key is specified as comment, there will not always be an email there which will fail every bark verification!

… key bates are second

… key

mirisbowring · 2024-07-02T12:17:26Z

@dainnilsson @elibon99 could you please have a look on the proposed bug fixes? We want to introduce this into our development flow but those bugs are currently showstoppers.

elibon99 · 2024-07-02T15:04:09Z

Hi @mirisbowring!

We are happy to hear that you are interested in incorporating GitBark into your development workflow. Thank you for your detailed feedback and proposed changes. Here are my thoughts on your suggestions:

1. Relative Paths

gitbark-core/bark_core/signatures.py

Line 355 in 6bb3f2d

with open(identifier, "rb") as f:

Since this plugin reads a possible public key from gitconfig, this line resulted in an Error:
[Errno 2] No such file or directory: '~/.ssh/id_ed25519.pub'
To fix this issue I propose to use os.path.expanduser to fix this common mistake:

Nice catch! Your suggestion with os.path.expanduser looks good and we are happy to merge this fix.

2. False starting index

During the __init__ of the SshKey you are splitting the read public key to extract the type and the key itself. But during the while iteration you start at 1 instead of 0 which results in an infinite loop until python reaches an out of index since i > len(parts) is never catched.

My proposed fix: Start with index 0

https://github.com/mirisbowring/gitbark-core/blob/d30c7ed2fcd6c0d172f4f7cd7af6433ef3553398/bark_core/signatures.py#L325

3. SSH comment interpreted as mail

While testing the bark verify i experienced a behaviour, that ssh signatures (though they are valid) are marked as invalid (without a reason). I did a deep dive into the code and discovered the following problem:

gitbark-core/bark_core/signatures.py

Line 324 in 6bb3f2d

self._emails = parts[0].decode().split(",")

The line above ready the first part of the public key as email which will NEVER work since the first part of the public key is always the keyspec like ssh_ed25519. Therefor, the mail Validation in L342 will always fail:

gitbark-core/bark_core/signatures.py

Line 342 in 6bb3f2d

if email not in self._emails:

To fix this issue, i set the index of the parts list to 2 (remember the format: keyspec_pubkey_comment):

https://github.com/mirisbowring/gitbark-core/blob/433326c1f1d256260047afa821fdafd7b731d312/bark_core/signatures.py#L324

The reason for why you are experiencing the above issues (2 & 3) is because we require SSH public keys to adhere to the format used in the ALLOWED SIGNERS file. In the context of GitBark, this means that a SSH public key file must contain the following space-separated fields: principals (comma-separated list of authorized email addresses), options (optional), keytype, base64-encoded key. This is why we read the first part of the public key file as the list of authorized emails:

gitbark-core/bark_core/signatures.py

Line 324 in 6bb3f2d

self._emails = parts[0].decode().split(",")

We understand that this requirement is not well-documented, and we appreciate your effort in identifying this problem. GitBark is still a work in progress, and we are continually striving to improve it as time allows.

As you've already discovered, parsing an SSH public key from an identifier given by gitconfig during bark setup may fail due to an incorrect public key file format. Until this is addressed, we recommend manually adding the public key in .bark/pubkeys with the correct format.

mirisbowring · 2024-07-02T15:33:25Z

Ah, that makes sense. In this case, wouldn't it make more sense to parse the gitconfig for the authorized_signers file (if configured)?

elibon99 · 2024-07-03T14:33:12Z

Ah, that makes sense. In this case, wouldn't it make more sense to parse the gitconfig for the authorized_signers file (if configured)?

I understand your point. However, when parsing a public key from gitconfig, our assumption is that the user wants to add just their own public key to the repository. Currently, this does not work for SSH public keys due to the invalid format, so a fix is needed in this area.

The authorized_signers file typically contains multiple public keys, which is why we are not using it in this context. However, enabling users to check in authorized_signers files within the repository (i.e., a file that lists multiple public keys) is a feature we are planning for future updates. Currently, each public key maps to its own file.

mirisbowring · 2024-07-03T15:39:52Z

I think, having pubkey files per user is not a bad idea for the following reasons:

depending on the used crypto algo they cen be relatively large
it would be possible to e.g. protect the keys differently. e.g. a maintainer pub key cannot be removed by any developer while developers can remove other developer keys...

For the initial import of public Keys it would be great if the user can select which public keys from authorized_signers he wants to include :)

I understand your point. However, when parsing a public key from gitconfig, our assumption is that the user wants to add just their own public key to the repository. Currently, this does not work for SSH public keys due to the invalid format, so a fix is needed in this area.

Probably there is a need for an additional function that gets called if the key is read from gitconfig and else the default behaviour steps in?

elibon99 · 2024-07-04T12:57:08Z

Probably there is a need for an additional function that gets called if the key is read from gitconfig and else the default behaviour steps in?

Yes! A simple way to accomplish this would be to parse the keytype and the base64-encoded key from the public key file pointed to by git config user.signingKey (e.g. ~/.ssh/id_ed25519.pub), and use git config user.email as the email address. Then we can use these values to construct a public key file that adheres to the ALLOWED SIGNERS format.

Additionally this commit introduces a mechanism to check if the configured signing key is an ssh key. If true it creates a tmp pubkey file that contains the user.email and user.signingKey from .gitconfig to form a pubkey with a valid format according to authorized_signers file.

mirisbowring · 2024-07-05T08:36:16Z

Ok, I reverted the initial changes with the index of the email for the public ssh keys.

Additionally i modified the get_pubkey_from_git function to check whether the gpg.format = ssh was set (meaning that ssh key is used for signing). If true, it creates a temporary file with

user.email user.signingKey

After adding the key, this temporary pubkey file gets deleted again.

mirisbowring · 2024-07-17T07:47:10Z

@elibon99 any chance this gets merged soon? 😊

elibon99 · 2024-08-05T10:44:41Z

bark_core/signatures.py

@@ -385,6 +387,17 @@ def get_authorized_pubkeys(

 def get_pubkey_from_git() -> Optional[Pubkey]:
    identifier = cmd("git", "config", "user.signingKey", check=False)[0]
+    gpg_format = cmd("git", "config", "gpg.format", check=False)[0]
+    # check if signingKey is ssh
+    if gpg_format and gpg_format == "ssh":


There's no need to parse and check gpg_format if identifier is None. I think it would be better to get gpg_format and perform the check here: https://github.com/YubicoLabs/gitbark-core/pull/21/files#diff-398bcc9bee5bed4a56368526a8c80e787bbbc105bb6b064fd6d7339b54413a3fR401.

elibon99 · 2024-08-05T10:44:44Z

bark_core/signatures.py

+        identifier = os.path.expanduser(identifier) # expand path if relative
+        with open(identifier, 'r') as file: # read public key
+            pubkey = file.read()
+        pubkey_tmp = f"{email} {pubkey}" # join email & pubkey to create correct format


Instead of creating a temporary file with the correct format, we could return an SshKey object directly: SshKey(f"{email} ".encode() + pubkey.read()).

This is an awesome idea! Will try it if i have some free time.

elibon99

Sorry for the late response. I had a look at the changes and in general it looks good. I have a few minor thoughts and comments. Please have a look at them

mirisbowring · 2024-09-03T09:49:00Z

Hi @elibon99 finally, i had time to implement the proposed changes!

elibon99 · 2024-09-05T11:32:16Z

bark_core/signatures.py

+        if gpg_format and gpg_format == "ssh":
+            email = cmd("git", "config", "user.email", check=False)[0]
+            identifier = os.path.expanduser(identifier) # expand path if relative
+            with open(identifier, 'r') as file: # read public key


Change file reading mode from 'r' to 'rb'.

You will have to remove pubkey.encode(), since pubkey is now already in bytes.

elibon99 · 2024-09-07T13:34:23Z

Thanks for your contribution! This has now been merged.

mirisbowring added 3 commits July 1, 2024 16:55

Fix issue when relative paths to public keys are configured in gitconfig

92a5793

Fixed index starting point since ssh-key spec is always first and the…

d30c7ed

… key bates are second

Fix false reference in split parts to get email (comment) from public…

433326c

… key

elibon99 reviewed Aug 5, 2024

View reviewed changes

elibon99 requested changes Aug 5, 2024

View reviewed changes

Move SSH-Key check after identifier verification

eba8262

elibon99 reviewed Sep 5, 2024

View reviewed changes

mirisbowring added 2 commits September 6, 2024 11:13

Using "rb" mode instead of "r" mode to read the key

968d296

Don't encode the pubkey twice while reading

293708f

mirisbowring requested a review from elibon99 September 7, 2024 13:19

elibon99 approved these changes Sep 7, 2024

View reviewed changes

elibon99 merged commit c5811aa into YubicoLabs:main Sep 7, 2024
1 check passed

elibon99 mentioned this pull request Sep 9, 2024

[BUG]: Unexpected Error if no public key present in repo #20

Closed

mirisbowring deleted the feature/fix-20 branch September 10, 2024 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/fix 20 #21

Feature/fix 20 #21

mirisbowring commented Jul 1, 2024 •

edited

Loading

mirisbowring commented Jul 2, 2024

elibon99 commented Jul 2, 2024

1. Relative Paths

2. False starting index

3. SSH comment interpreted as mail

mirisbowring commented Jul 2, 2024

elibon99 commented Jul 3, 2024

mirisbowring commented Jul 3, 2024 •

edited

Loading

elibon99 commented Jul 4, 2024

mirisbowring commented Jul 5, 2024

mirisbowring commented Jul 17, 2024

elibon99 Aug 5, 2024

elibon99 Aug 5, 2024

mirisbowring Aug 5, 2024

elibon99 left a comment

mirisbowring commented Sep 3, 2024

elibon99 Sep 5, 2024

mirisbowring Sep 6, 2024

elibon99 Sep 6, 2024

mirisbowring Sep 6, 2024

elibon99 commented Sep 7, 2024

Feature/fix 20 #21

Feature/fix 20 #21

Conversation

mirisbowring commented Jul 1, 2024 • edited Loading

1. Relative Paths

2. False starting index

3. SSH comment interpreted as mail

mirisbowring commented Jul 2, 2024

elibon99 commented Jul 2, 2024

1. Relative Paths

2. False starting index

3. SSH comment interpreted as mail

mirisbowring commented Jul 2, 2024

elibon99 commented Jul 3, 2024

mirisbowring commented Jul 3, 2024 • edited Loading

elibon99 commented Jul 4, 2024

mirisbowring commented Jul 5, 2024

mirisbowring commented Jul 17, 2024

elibon99 Aug 5, 2024

Choose a reason for hiding this comment

elibon99 Aug 5, 2024

Choose a reason for hiding this comment

mirisbowring Aug 5, 2024

Choose a reason for hiding this comment

elibon99 left a comment

Choose a reason for hiding this comment

mirisbowring commented Sep 3, 2024

elibon99 Sep 5, 2024

Choose a reason for hiding this comment

mirisbowring Sep 6, 2024

Choose a reason for hiding this comment

elibon99 Sep 6, 2024

Choose a reason for hiding this comment

mirisbowring Sep 6, 2024

Choose a reason for hiding this comment

elibon99 commented Sep 7, 2024

mirisbowring commented Jul 1, 2024 •

edited

Loading

mirisbowring commented Jul 3, 2024 •

edited

Loading