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.

Sign up for GitHub

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

Jump to bottom

Adding URI SAN support #63

Merged
merged 11 commits into from
Oct 17, 2018
Merged

Adding URI SAN support #63

merged 11 commits into from
Oct 17, 2018

Conversation

mweissbacher
Copy link
Contributor

No description provided.

@CLAassistant
Copy link

CLAassistant commented Oct 16, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@mweissbacher
Copy link
Contributor Author

This PR depends on Go 1.11

mcpherrinm
mcpherrinm suggested changes Oct 16, 2018
View reviewed changes
pkix/csr.go Show resolved Hide resolved
@mcpherrinm
Copy link
Contributor

I tested a bit, and it seems like the CSRs work, but we'll need to add support to certstrap sign too.

also, it's pretty weird to put a URI in the Common Name. I think that likely isn't what anyone will intend. Maybe we should make -common-name required if no --domain flag is passed too

@mweissbacher
Copy link
Contributor Author

also, it's pretty weird to put a URI in the Common Name. I think that likely isn't what anyone will intend. Maybe we should make -common-name required if no --domain flag is passed too

How about IPs? We could just remove URIs (and IPs?) from the naming switch statement in cmd/request_cert.go and it'll fail automatically if no domain or common name are provided.

mcpherrinm
mcpherrinm reviewed Oct 17, 2018
View reviewed changes
README.md Outdated
@@ -66,9 +66,9 @@ Created out/Alice.key
Created out/Alice.csr
```

certstrap requires at least one of `-common-name`, `-ip`, or `-domain` flags to be set in order to generate a certificate signing request. The CN for the certificate will be found from these fields.
certstrap requires at least one of `-common-name`, `-ip`, `-domain`, or `-uri` flags to be set in order to generate a certificate signing request. The CN for the certificate will be found from these fields.
Copy link
Contributor

Choose a reason for hiding this comment

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

This line isn't quite accurate. We require at least one of -common-name or -domain now

@mcpherrinm
Copy link
Contributor

One minor nit in the README.md, otherwise looks good now

@mweissbacher mweissbacher merged commit ff11e75 into square:master Oct 17, 2018
@mweissbacher mweissbacher deleted the Adding-support-for-URI-SANs branch October 17, 2018 23:00
@mbyczkowski mbyczkowski mentioned this pull request Sep 18, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcpherrinm mcpherrinm mcpherrinm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mweissbacher @CLAassistant @mcpherrinm