Skip to content

Expose verified host certificates through Session - #1119

Open
fornwall wants to merge 1 commit into
mwiede:masterfrom
fornwall:expose-host-certificate
Open

fornwall wants to merge 1 commit into
mwiede:masterfrom
fornwall:expose-host-certificate

Conversation

@fornwall

Copy link
Copy Markdown
Contributor

Summary

Expose the verified OpenSSH host certificate through Session#getHostKeyCertificate().

Successful certificate verification currently returns from Session#checkHost before doCheckHostKey, leaving Session#getHostKey() unset and the certificate inaccessible to callers. This change:

  • records the underlying public key in Session#getHostKey();
  • retains the verified certificate in Session#getHostKeyCertificate();
  • makes OpenSshCertificate and its read accessors public.

getHostKeyCertificate() remains null for plain host-key authentication and when certificate verification fails but host_certificate_to_key_fallback accepts the underlying key.

Tests

./mvnw -B -DskipITs=false -Dit.test=HostCertificateIT verify

  • 487 unit tests passed
  • 11 HostCertificateIT tests passed
  • formatting, import sorting, and forbidden-API checks passed

Disclaimar: Created with claude code and opus 5, reviewed by me.

Record the plain host key and verified OpenSSH certificate after
certificate-based host authentication succeeds. Make OpenSshCertificate
and its read accessors public so applications can inspect the certificate.

Keep the certificate unset when plain-key fallback is used, and cover both
certificate and plain-key authentication paths.
@sonarqubecloud

Copy link
Copy Markdown

@norrisjeremy

Copy link
Copy Markdown
Contributor

What's the purpose of this?
We don't currently return the verified host key, so I'm not sure I understand the reason to return the verified host certificate?

@norrisjeremy

Copy link
Copy Markdown
Contributor

What's the purpose of this? We don't currently return the verified host key, so I'm not sure I understand the reason to return the verified host certificate?

Nvm, let me think on this some more.
I'm not sure I'm comfortable with the approach taken here.

* @return the verified host certificate, or {@code null} if none was used
* @see #getHostKey()
*/
public OpenSshCertificate getHostKeyCertificate() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am opposed to making the OpenSshCertificate class a part of the public API.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks for the feedback! Let me know if you think the certificate information can be exposed in some other way and I'll try that out. Otherwise I'll close this PR as a wont do.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to think through this some more to try and think of a solution that wouldn't leak the internal OpenSshCertificate class.

@norrisjeremy

Copy link
Copy Markdown
Contributor

FYI, I haven't forgotten about this and am working on an alternative solution.

@norrisjeremy

Copy link
Copy Markdown
Contributor

Hi @fornwall,

Just wanted to update you that I haven't forgotten about this.
I'm currently waiting on a clarification from the author of draft-ietf-sshm-cert on how a few fields are structured.

Thanks,
Jeremy

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.

2 participants