Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix order in which fingerprints are evaluated #415

Merged
merged 4 commits into from
Jun 6, 2023
Merged

Conversation

michaeljguarino
Copy link
Member

Summary

this needs to be done on the key generated from the provider, not at the start

Test Plan

tbd

Checklist

  • If required, I have updated the Plural documentation accordingly.
  • I have added tests to cover my changes.
  • I have added a meaningful title and summary to convey the impact of this PR to a user.
  • I have added relevant labels to this PR to help with categorization for release notes.

this needs to be done on the key generated from the provider, not at the start
@michaeljguarino michaeljguarino added the bug-fix This pull request fixes a bug label Jun 2, 2023
@zreigz
Copy link
Member

zreigz commented Jun 5, 2023

@michaeljguarino This solution also doesn't work. We still compare two different encryption keys. The SHA is generated from ~/.plural/key When we import some repo from another user the key is always different than ours. The validation was added to check if the current encryption key is different from the one that was used during repo init. The sharing scenario will always fail in this case. In my opinion, we should add a user email address to the .keyid file and validate the key when the emails are equal

@zreigz
Copy link
Member

zreigz commented Jun 5, 2023

Also plural init can't decrypt the repo

@michaeljguarino
Copy link
Member Author

This should extract the key from the crypto provider, in the case of sharing that will be the key decrypted from w/in .plural-crypt. Have you tested it to confirm it doesn't work? If not we probably just need to tweak some of the validation

@michaeljguarino
Copy link
Member Author

@zreigz fixed the bug with latest commit

@zreigz
Copy link
Member

zreigz commented Jun 6, 2023

@michaeljguarino I have fixed a few issues and tested it again. Adde e2e test for repo sharing passed. PTAL and we can merge it

@michaeljguarino
Copy link
Member Author

@zreigz lgtm (can't approve though since it's my PR i think)

@zreigz zreigz merged commit 885ff98 into main Jun 6, 2023
@michaeljguarino michaeljguarino deleted the fix-fingerprint branch June 6, 2023 15:03
michaeljguarino added a commit that referenced this pull request Aug 28, 2024
* Fix order in which fingerprints are evaluated

this needs to be done on the key generated from the provider, not at the start

* need to base32 encode instead of base64

* fix when .keyid doesn't exist

* fix error handling

---------

Co-authored-by: Lukasz Zajaczkowski <zreigz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix This pull request fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants