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

Add testing/certutil/ package for TLS test support #189

Merged
merged 5 commits into from
Mar 21, 2024

Conversation

AndersonQ
Copy link
Member

What does this PR do?

Adds the testing/certutil/ package providing on-the-fly generation of TLS certificate and root CA and a CLI to generate them as well.

Why is it important?

The Elastic Agent and Bats need to configure TLS for testing. This provide an easy way to generate the needed certificates without duplicating code in the Elastic Agent and Beats repo

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have added tests that prove my fix is effective or that my feature works

Related issues

`testing/certutil/certutil` provides functionality to generate root CA and child certificates for use in tests
It can also be used as a CLI
@AndersonQ AndersonQ added the enhancement New feature or request label Mar 11, 2024
@AndersonQ AndersonQ self-assigned this Mar 11, 2024
@AndersonQ AndersonQ requested a review from a team as a code owner March 11, 2024 15:41
@AndersonQ AndersonQ requested review from ycombinator, leehinman, michalpristas and blakerouse and removed request for a team, ycombinator and leehinman March 11, 2024 15:41
@leehinman
Copy link
Contributor

I am strongly in favor of generating the CA & certs as needed for testing. But do we need to write our own utility?

https://github.com/cloudflare/cfssl

Already does all of this, is go so we can embed in our tests, and has been around a while so most of the subtle X509 considerations have been addressed.

Downside is that we don't build up as much X509 knowledge in our team. Thoughts?

@pierrehilbert pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Mar 11, 2024
@AndersonQ
Copy link
Member Author

I am strongly in favor of generating the CA & certs as needed for testing. But do we need to write our own utility?

https://github.com/cloudflare/cfssl

Already does all of this, is go so we can embed in our tests, and has been around a while so most of the subtle X509 considerations have been addressed.

Downside is that we don't build up as much X509 knowledge in our team. Thoughts?

I was having a look at it. It feels overkill and I'd bet getting used to use it would not be much different than using what this PR adds. Also as you pointed, by building it ourselves we get some more knowledge in the team about how all the moving pieces get together.

Lastly, it's done already, the beats use a similar code that can, and should, be replaced by this new package. Just the work to replace this PR and instead use github.com/cloudflare/cfssl would not be worth.

NotBefore: notBefore,
NotAfter: notAfter,
KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
ExtKeyUsage: []x509.ExtKeyUsage{
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is really a root CA it should not have ClientAuth & ServerAuth as extended key usages. CAs are only supposed to sign certificates.

certTemplate := &x509.Certificate{
DNSNames: []string{name},
IPAddresses: ips,
SerialNumber: big.NewInt(1658),
Copy link
Contributor

Choose a reason for hiding this comment

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

Every child cert shouldn't have the same serial number. Ideally the CA is keeping track of this and providing unique serial numbers. If you don't want to track that for testing, just using a random int over a "large" space is good enough.

// - the certificate and private key as a tls.Certificate
//
// If any error occurs during the generation process, a non-nil error is returned.
func GenerateChildCert(name string, ips []net.IP, caPrivKey crypto.PrivateKey, caCert *x509.Certificate) (*tls.Certificate, Pair, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't have to change this, but as an FYI. This is "unusual" from a CA perspective, usually a client generates a "To Be Signed Certificate Request" and sends that to the CA. In go that would be https://pkg.go.dev/crypto/x509@go1.22.1#CertificateRequest. If you look inside that struct you see it just contains the Public Key of the requester. The CA never "sees" the private key.

@AndersonQ
Copy link
Member Author

as discussed directly with @leehinman, the child certificate generation will be improved on another PR and use the Certificate Request

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @AndersonQ

@AndersonQ AndersonQ merged commit d221ab4 into elastic:main Mar 21, 2024
6 checks passed
@AndersonQ AndersonQ deleted the 2247-mtls branch March 21, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants