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

Allow setting CN and validity days on certificate generation #6

Merged

Conversation

lucasmrod
Copy link
Contributor

This allow setting a custom CN and validity days on GET /v1/tokenpki/{name}. Such API was always using default values CN=depserver and days=1.

See #5 (comment).

@jessepeterson
Copy link
Member

In general this is great! Have you noticed an actual problem with the validity date? I recall using an expired certificate and it working on the ABM portal but can't remember clearly: I think the certificate is used purely as a vessel for the public key (for encryption) rather than any of its other attributes. 🤷

The only change I'd make is not requiring the parameters in the tools/shell script. They could be in the shell script - just not required parameters ($1 etc). I want to keep those as simple as possible for end users. The API docs (thanks!) specify the variables so for integrations that could work.

Thanks again!

@jessepeterson
Copy link
Member

Actually thinking about it again - if those params are omitted they'll just be blank as they're passed on. Perhaps then just the QuickStart docs can omit them :)

@lucasmrod
Copy link
Contributor Author

Have you noticed an actual problem with the validity date? I recall using an expired certificate and it working on the ABM portal but can't remember clearly: I think the certificate is used purely as a vessel for the public key (for encryption) rather than any of its other attributes. 🤷

Oh! I would expect any tooling to fail if a certificate is expired :) (And thus I thought it would be handy to allow people to configure more than one day to allow for more flexibility, e.g. the user generating the certificate might belong to different team than user uploading it to ABM.)

The only change I'd make is not requiring the parameters in the tools/shell script. They could be in the shell script - just not required parameters ($1 etc). I want to keep those as simple as possible for end users. The API docs (thanks!) specify the variables so for integrations that could work.

Good idea. I've set default arguments in the tools and amended the docs/quickstart.md.

Let me know if that works.

Copy link
Member

@jessepeterson jessepeterson left a comment

Choose a reason for hiding this comment

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

In addition to the other comments had a few code questions/comments. Thanks for this work!

cmd/deptokens/main.go Outdated Show resolved Hide resolved
cmd/deptokens/main.go Outdated Show resolved Hide resolved
cmd/deptokens/main.go Outdated Show resolved Hide resolved
docs/openapi.yaml Show resolved Hide resolved
docs/operations-guide.md Show resolved Hide resolved
http/api/tokenpki.go Outdated Show resolved Hide resolved
docs/openapi.yaml Outdated Show resolved Hide resolved
tokenpki/cert.go Show resolved Hide resolved
tools/cfg-get-cert.sh Outdated Show resolved Hide resolved
Copy link
Member

@jessepeterson jessepeterson left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for the changes (and sorry for the delay!)

@jessepeterson jessepeterson merged commit a7538c5 into micromdm:main Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants