Improve `TlsValidator.HasSelfSignedCert()`

[g-gaston]

Coming from https://github.com/aws/eks-anywhere/pull/1356#discussion_r819556055

What would you like to be added: ~~This code in TlsValidator.HasSelfSignedCert() is not very accurate. Generally a certificate is self signed if the issuer and subject are the same but I think there are more complicated semantics to self-signed certs so using x509 lib would be good.~~

~~We ought to verify its self signed by: (1) connect to acquire the cert ignoring any cert verification errors; (2) add the cert to a CertPool; (3) Use the CertPool as an option to Certifiate.Verify().~~

~~See https://stackoverflow.com/a/63317898~~

After a deeper examination of the code (thanks to @BrowduesMan85!), it seems like the method is doing what it should but its name is misleading.

Rename this method to IsSignedByUnknownAuthority and update caller and mocks. It also looks like the method might need a bit of cleanup (for example, close the connection after we are done with it).

[chrisdoherty4]

@vivek-koppuru I think you may have authored this. Are you able to speak to its intent?

[vivek-koppuru]

@chrisdoherty4 I actually just refactored this code but not introduce this specific functionality. I think @abhinavmpandey08 or @ptrivedi might be able to speak more to it.

[browdues]

Hello @g-gaston, I picked up this issue in order to begin familiarizing myself with the code base. I implemented the check you suggested and during testing, I discovered that it will falsely indicate that a CA cert is self signed.

Although baffled at first, upon examining the cert.Verify(opts) method, I found that when no intermediate was supplied, and the cert pool contained the cert, the Verify method returned a nil error, as well as a cert chain containing only the cert.

Since this holds true whether or not the cert is self signed or signed by a CA, it won’t work for this use case.

Another approach is contained here. This approach checks (1) that the Issuer matches the Subject and (2) if an AuthorityKeyId exists, ensure it matches the SubjectKeyId. These fields are readily available on the x509 certificate type, and could be easily implemented.

Any issue taking this approach?

[g-gaston]

Hey @BrowduesMan85! Thanks a lot for looking into this! You are completely right, the proposed solution won't work, apologies for that. Also, what you propose as an alternative solution sounds good to me.

That said, when I read you comment, I went and took a step back and look at the code that is calling this HasSelfSignedCert method.

I think this method was misnamed and hence we are trying to change its implementation to do what the name of the method says and not what the calling code (there is only one place) expects us to do.

This is the calling code: https://github.com/aws/eks-anywhere/blob/51efb63a7a35dcae8d78ba7272cd21fbbf28a262/pkg/validations/cluster.go#L12-L45

The purpose of calling HasSelfSignedCert seems to be to check if passing the CA cert should be required or not. Which makes me think that we are not trying to check if the server cert is self signed but if the server cert is signed by a private entity (CA or not). Whether the server cert is self-signed or not I don't think it matters, what matters is if we need a custom cert to validate it's signature or not.

What I think we should do is remove HasSelfSignedCert and write something like IsSignedByUnknownAuthority. And then use this new method from ValidateCertForRegistryMirror.

Would you be ok taking up on this new task? I can update the description and repurpose this same issue.

Thanks again!

[browdues]

Hi @g-gaston, catching up after being out last week. Thanks for the response and yes I'd be glad to take on the task!

That said, after reading your comments and looking at the code, it almost seems like the code in place does just what is needed - determining if the cert is signed by an unknown authority.

https://github.com/aws/eks-anywhere/blob/4b65f85cf04cb78e93544308147d754e42ba5dba/pkg/crypto/validator.go#L22-L37

I could be completely off base, so please update the description and repurpose this is if you had something else in mind.

[g-gaston]

@BrowduesMan85 agreed, that's what it does.

Then we should probably just rename the method so it says it does what it's actually doing.

I'll repurpose this issue to do that but don't feel like you need to resolve it, renaming a method is not necessarily a good first issue. If you are not interested, feel free to unassign yourself and we can find something a bit more fun if you want 🙂

[browdues]

@g-gaston Should anything else arise, please put my name on it! Although this might not be the most interesting of issues, I'm still glad to see it through, if only as a means to begin building contributions.

[g-gaston]

@BrowduesMan85 Awesome! Updated the description and assigned to you!