Actually always load .pub file if present

[egfx-notifications]

Fixes #348

[dlech]

Based on https://github.com/dlech/KeeAgent/issues/348#issuecomment-1266935972, it sounds like we should modify the Read() method to always use the publicKey arg if provided rather than this approach.

[egfx-notifications]

Based on #348 (comment), it sounds like we should modify the Read() method to always use the publicKey arg if provided rather than this approach.

We could modify the Read() method, but I don't know if changing that behavior from public key as fallback to always use public key would introduce problems elsewhere in the code. Checking where that method is used in KeeAgent could be done, but I don't know if SshAgentLib is used in other projects and changing this behavior should thus be considered a breaking change for the library.

I see these options to resolve this in KeeAgent only if we want to be on the safe side (code just hacked together, should be rechecked if we decide for one of the options):

• always load the public key separately first, then execute Read() with the public key (this will take care of old keys without public key in the private key), then execute withPublicKey() (which will be a redundant operation if the private key was not encrypted, but makes sure that an existing pubkey attachment is always preferred)

  case EntrySettings.LocationType.Attachment: {
          if (string.IsNullOrWhiteSpace(settings.Location.AttachmentName)) {
            throw new NoAttachmentException();
          }
  
          var attachment = binaries.Get(settings.Location.AttachmentName);
  
          if (attachment == null) {
            throw new NoAttachmentException();
          };
  
          SshPublicKey publicKey = null;
  
          var publicKeyAttachment = binaries.Get(settings.Location.AttachmentName + ".pub");
  
          if (publicKeyAttachment != null) {
            publicKey = SshPublicKey.Read(new MemoryStream(publicKeyAttachment.ReadData()));
          }
  
          // if a separate public key attachment exists, provide it to be used if the private key does not contain the public key
          var privateKey = SshPrivateKey.Read(new MemoryStream(attachment.ReadData()), publicKey);
  
          // if a separate public key attachment exists, prefer it over the public key that may be included in the private key
          // this enables display of public key comments if the private key is encrypted
          privateKey = privateKey.WithPublicKey(publicKey);
  
          var certificateAttachment = binaries.Get(settings.Location.AttachmentName + "-cert.pub");
  
          if (certificateAttachment != null) {
            privateKey = privateKey.WithPublicKey(SshPublicKey.Read(new MemoryStream(certificateAttachment.ReadData())));
          }
  
          return privateKey;
        }

• catch the PublicKeyRequiredException and try again with separate public key, rethrow if no separate public key exists (no redundant operations, but code flow less clear)

  case EntrySettings.LocationType.Attachment: {
          if (string.IsNullOrWhiteSpace(settings.Location.AttachmentName)) {
            throw new NoAttachmentException();
          }
  
          var attachment = binaries.Get(settings.Location.AttachmentName);
  
          if (attachment == null) {
            throw new NoAttachmentException();
          };
  
          SshPrivateKey privateKey = null;
          var publicKeyLoadError = false;
  
          try {
            privateKey = SshPrivateKey.Read(new MemoryStream(attachment.ReadData()));
          }
          catch (SshPrivateKey.PublicKeyRequiredException) {
            publicKeyLoadError = true;
          }
  
          var publicKeyAttachment = binaries.Get(settings.Location.AttachmentName + ".pub");
  
          // if a separate public key attachment exists, prefer it over the public key that may be included in the private key
          // this enables display of public key comments if the private key is encrypted
          if (publicKeyAttachment != null) {
            privateKey = privateKey.WithPublicKey(SshPublicKey.Read(new MemoryStream(publicKeyAttachment.ReadData())));
            publicKeyLoadError = false;
          }
  
          var certificateAttachment = binaries.Get(settings.Location.AttachmentName + "-cert.pub");
  
          if (certificateAttachment != null) {
            privateKey = privateKey.WithPublicKey(SshPublicKey.Read(new MemoryStream(certificateAttachment.ReadData())));
            publicKeyLoadError = false;
          }
  
          if (publicKeyLoadError == true) {
            throw new PublicKeyRequiredException();
          }
  
          return privateKey;
        }

[dlech]

but I don't know if SshAgentLib is used in other projects

None that I know of, so breaking changes are not a problem.

[egfx-notifications]

but I don't know if SshAgentLib is used in other projects

None that I know of, so breaking changes are not a problem.

Alright, so I guess we'll go with modifying Read(). Do you want to work on this or should I make a PR?

[dlech]

Happy to review a PR. It is quite low priority for me to do the work myself.

[egfx-notifications]

I'll keep this PR as a draft until the changes in SshAgentLib have been tested and merged.

[egfx-notifications]

I created a PR for SshAgentLib (https://github.com/dlech/SshAgentLib/pull/22)

The commits in this PR use my fork of SshAgentLib as submodule to verify that the fix works as intended. If you merge my SshAgentLib PR, I can either change this PR to work with the actual SshAgentLib submodule or we can close this PR and you just update the submodule as no further changes to KeeAgent would be necessary anyway.

[dlech]

Thanks for the work on this. https://github.com/dlech/KeeAgent/issues/348#issuecomment-1279830288 suggested that this might not fully fix the issue, so I went with a different solution.

Let me know if https://github.com/dlech/KeeAgent/suites/8800273334/artifacts/400143145 fixes the issue.

[egfx-notifications]

I think @jaxFF may not have actually tested with https://github.com/dlech/KeeAgent/suites/8648554769/artifacts/388861733 from this PR's artifacts as I have ed25519 keys with spaces in the comments as well and those work fine. I think they may have just tested the most recent release assuming that the PR was already included in it.

I did test your fix, however, and as far as I can tell so far it fixes the issue as well.

[jaxFF]

I think @jaxFF may not have actually tested with https://github.com/dlech/KeeAgent/suites/8648554769/artifacts/388861733 from this PR's artifacts as I have ed25519 keys with spaces in the comments as well and those work fine. I think they may have just tested the most recent release assuming that the PR was already included in it.

I did test your fix, however, and as far as I can tell so far it fixes the issue as well.

Yeah, it's clear to me now I was on a slightly older version. I assumed this PR was included after a quick, but neglectful glance at the changelog.

I tested @dlech's fix, as well as this feature branch, and they both fix the initial issue for me. Appreciate your guys' work & support on this repository, and I imagine it's nice to harden this path through 55241d92d8acb6ffd0fc7ce6991d44f2dd933b69.