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

trim leaf cert and key when generated #10417

Closed
wants to merge 5 commits into from
Closed

Conversation

dhiaayachi
Copy link
Collaborator

this is fix #8178

When we generate a key the tls lib add a trailing end of line to each generated certificate. This PR is to remove the trailing newline from the certPEM and private key

@github-actions github-actions bot added the theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies label Jun 16, 2021
@vercel vercel bot temporarily deployed to Preview – consul June 16, 2021 19:25 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging June 16, 2021 19:25 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging June 16, 2021 19:27 Inactive
@vercel vercel bot temporarily deployed to Preview – consul June 16, 2021 19:27 Inactive
@dnephin
Copy link
Contributor

dnephin commented Jun 18, 2021

Thank you for working on this! I just had another read of #8178 to refresh my memory of the issue.

My understanding matches what was said here: #8178 (comment)

I believe the problem is actually the inverse of what is implemented in the PR, we should be adding a newline somewhere when one is missing, not removing newlines.

The spec wants a single trailing newline after the cert, and an extra one to separate multiple certs.

@dhiaayachi
Copy link
Collaborator Author

dhiaayachi commented Jun 18, 2021

I think we should have a conversation with @mikemorris as my understanding of the spec he quoted in the issue is no newline at the end and a single new line between certs.
I did my own research and you are right, I will work on making sure that we always have a trailing new line (from all the providers)

@vercel vercel bot temporarily deployed to Preview – consul June 21, 2021 14:34 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging June 21, 2021 14:34 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging June 21, 2021 14:46 Inactive
@vercel vercel bot temporarily deployed to Preview – consul June 21, 2021 14:46 Inactive
@dhiaayachi dhiaayachi requested a review from dnephin June 21, 2021 16:15
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Nice! Just a couple small suggests

Comment on lines +442 to +445
cert := strings.TrimSuffix(buf.String(), "\n")

// Set the response
return buf.String(), nil
return fmt.Sprintf("%s\n", cert), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

as we chatted, using HasSuffix may avoid the allocation in the common case.

@@ -438,8 +439,10 @@ func (c *ConsulProvider) Sign(csr *x509.CertificateRequest) (string, error) {
return "", fmt.Errorf("error encoding certificate: %s", err)
}

cert := strings.TrimSuffix(buf.String(), "\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a comment to say we don't expect a missing newline, but we are doing this because other providers had the problem and we want to be cautious.

@dhiaayachi
Copy link
Collaborator Author

This will be handled in #10411

@dhiaayachi dhiaayachi closed this Jun 22, 2021
@dhiaayachi dhiaayachi deleted the fix-leaf-ca-trailing branch July 15, 2021 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/v1/agent/connect/ca/leaf/web endpoint returns CertPEM without a trailing newline when using vault ca provider
2 participants