Skip to content

Adding URI SAN support #63

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
Merged
mweissbacher merged 11 commits into from
Oct 17, 2018
Merged

Adding URI SAN support #63

mweissbacher merged 11 commits into from
Oct 17, 2018

Conversation

@mweissbacher
Copy link
Contributor

@mweissbacher mweissbacher commented Oct 16, 2018

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

mweissbacher commented Oct 16, 2018

This PR depends on Go 1.11

mcpherrinm
mcpherrinm suggested changes Oct 16, 2018
View reviewed changes
@mcpherrinm
Copy link
Contributor

mcpherrinm commented Oct 16, 2018

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

mweissbacher commented Oct 17, 2018

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
```

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

@mcpherrinm mcpherrinm Oct 17, 2018

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

mcpherrinm commented Oct 17, 2018

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

1 more reviewer

@mcpherrinm mcpherrinm mcpherrinm approved these changes

Reviewers whose approvals may not affect merge requirements

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