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

implement reading comment from openssh private key #23

Merged
merged 3 commits into from
Feb 4, 2023

Conversation

topia
Copy link
Contributor

@topia topia commented Dec 1, 2022

Reading key comment from an openssh private key, Because it's encrypted, added callback on Decrypt.
https://github.com/openssh/openssh-portable/blob/4a1805d532616233dd6072e5cd273b96dd3062e6/PROTOCOL.key#L38-L39

See also: dlech/KeeAgent#375

@dlech
Copy link
Owner

dlech commented Dec 1, 2022

Thanks for the PR.

Instead of adding a callback for the comment, could we just add a property for it like we already have for all of the other info parsed from the private key?

@topia
Copy link
Contributor Author

topia commented Dec 1, 2022

Instead of adding a callback for the comment, could we just add a property for it like we already have for all of the other info parsed from the private key?

Thanks for your comment. I want to make sure which method you said; I think two options for that:

  1. drop callback for SshPrivateKey.Decrypt only
  • keep callback for internal DecryptFunc. it makes short diff and simple changes but might be confusing for implementors.
  1. drop callback for Decrypt and DecryptFunc
  • closure needs to keep a reference for SshPrivateKey to update its property, or maybe we can pass this to the delegate.

Also, How about the type of that property? I consider string? because I think it has three states - (not loaded/private key doesn't have any comment), (loaded but empty comment), (loaded and some comment).
Sometimes we want to distinguish between not loaded and loaded but no comment field for telling the caller a behavior of that property.

@dlech
Copy link
Owner

dlech commented Dec 1, 2022

Apparently, I didn't think about it enough. 😄

A property that we have to update doesn't seem right since we want the object to be immutable.

Instead of a callback in the decrypt function though, it seems like it would be simpler to just have an out paramter (or modify the return value to return a ValueTuple).

@topia
Copy link
Contributor Author

topia commented Dec 1, 2022

I started with the callback design to keep source-level compatibility for them.
But now I changed all callers for some reason. I'll consider the out parameter for that.

@codecov-commenter
Copy link

Codecov Report

Merging #23 (d12cce6) into master (1dcb0ed) will decrease coverage by 0.05%.
The diff coverage is 75.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
- Coverage   51.83%   51.79%   -0.05%     
==========================================
  Files          37       37              
  Lines        5533     5553      +20     
  Branches      503      504       +1     
==========================================
+ Hits         2868     2876       +8     
- Misses       2482     2493      +11     
- Partials      183      184       +1     
Impacted Files Coverage Δ
SshAgentLib/IAgent.cs 0.00% <0.00%> (ø)
SshAgentLib/Keys/OpensshPrivateKey.cs 75.48% <100.00%> (+0.48%) ⬆️
SshAgentLib/Keys/PemPrivateKey.cs 82.00% <100.00%> (+2.45%) ⬆️
SshAgentLib/Keys/PuttyPrivateKey.cs 79.60% <100.00%> (+0.49%) ⬆️
SshAgentLib/Keys/SshPrivateKey.cs 80.00% <100.00%> (ø)
SshAgentLib/WslSocket.cs 83.51% <0.00%> (-7.70%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dlech dlech merged commit 6809f3e into dlech:master Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants