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

Improve Subject Name handling for Conjur CA certificates #712

Merged
merged 6 commits into from Sep 11, 2018

Conversation

micahlee
Copy link
Contributor

@micahlee micahlee commented Aug 29, 2018

Closes #705
Closes #707

What does this pull request do?

This PR updates the Conjur CA certificates in 2 ways:

  1. The Common Name on the certificate is now given directly from the authenticated role (must be host), and not from the CSR. The common name has the format <account>:<ca-service-id>:host:<host-id>. A DNS alternative name is added for the leaf (right-most) portion of the slash separate host identifier. This is to support the existing demo.

  2. The certificate now includes a URI subject alternative name containing the SPIFFE Id for the role. This currently takes the form of spiffe://conjur/<account>/<ca-service-id>/host/<host-id>.

Where should the reviewer start?

The core changes are in app/domain/ca/certificate_authority.rb

How should this be manually tested?

The API cucumber tests include verification that these IDs are present and correct.

If you want to, you can use the demo (https://github.com/conjurdemos/misc-util/tree/master/demos/certificate-authority/mutual-tls) to generate a certificate. You can then use OpenSSL to view the contents to verify the subject names, e.g.:

openssl x509 -in certificate.crt -text -noout

Link to build in Jenkins (if appropriate)

https://jenkins.conjur.net/job/cyberark--conjur/job/ca_csr_subject/

Questions:

Does this have automated Cucumber tests?
Yes

Can we make a blog post, video, or animated GIF out of this?
Probably

Is this explained in documentation?
Not yet.

Does the knowledge base need an update?
No

@micahlee micahlee self-assigned this Aug 29, 2018
@ghost ghost added in progress labels Aug 29, 2018
@jvanderhoof
Copy link
Contributor

This code looks solid, but I think we should hold on merging until we run the host name format by a customer or two. I worry it'll be too restrictive.

@micahlee micahlee changed the base branch from ca_signing_service to master August 31, 2018 16:58
csr_cert
end

protected

def subject(role)
common_name = [
'conjur',
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about pulling this out? I'm not sure I'd love to need to put conjur into my host naming convention, particularly if all hosts were managed by Conjur. Without it, organizations are free to set account, service_id, and host id as they see fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm ok with that. I think we had settled on this because it roughly mirrored the SPIFFE ID, but there is no other reason I am aware of at this point. I will change it.

@micahlee micahlee changed the title WIP Improve Subject Name handling for Conjur CA certificates Improve Subject Name handling for Conjur CA certificates Sep 10, 2018
@codeclimate
Copy link

codeclimate bot commented Sep 10, 2018

Code Climate has analyzed commit 70456e7 and detected 5 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 5

View more on Code Climate.

@jvanderhoof jvanderhoof self-requested a review September 11, 2018 19:13
Copy link
Contributor

@jvanderhoof jvanderhoof left a comment

Choose a reason for hiding this comment

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

Looks good!

@jvanderhoof jvanderhoof merged commit 5b1537a into master Sep 11, 2018
@ghost ghost removed the in progress label Sep 11, 2018
@jvanderhoof jvanderhoof deleted the ca_csr_subject branch September 11, 2018 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants