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

increase authn:keygen_timeout for oc_erchef hab pkg #1579

Merged

Conversation

jeremymv2
Copy link
Contributor

Description

This is a very simple change to the oc_erchef Hab package default configuration.
We noticed that the keygen_timeout value was too small out of the box and would continually spam these warnings:

oc-erchef.default(O): [warning] {chef_keygen_cache,keygen_timeout}

Increasing to this proposed value yielded no adverse affects in our testing over the months and so I decided to just make it the new default 😄

Signed-off-by: Jeremy J. Miller [email protected]

@jeremymv2 jeremymv2 requested a review from a team October 2, 2018 16:04
@stevendanna
Copy link
Contributor

Increasing to this proposed value yielded no adverse affects in our testing over the months and so I decided to just make it the new default 😄

Bumping this is probablly fine, but 20 seconds seems very long. Is this testing something we can put in the pipeline and use it to choose a smaller value? Happy to just merge this as is though.

@jeremymv2
Copy link
Contributor Author

@stevendanna sure! I can spin up the container stack and test some smaller values out today at the same time I'm validating #1573 😄

@jeremymv2 jeremymv2 force-pushed the jeremymv2/chef_authn_hab_pkg_defaults branch from ad6e2cc to 30db7c6 Compare October 4, 2018 14:50
@jeremymv2
Copy link
Contributor Author

jeremymv2 commented Oct 4, 2018

@stevendanna I consistently got timeouts with a value of 2000, therefore I'm thinking doubling that and rounding up to 5s is reasonable. I have squashed and pushed up.

@stevendanna
Copy link
Contributor

5 seconds is exactly in the realm of what I would expect. I'm actually pretty surprised we don't see this fail more often at 1 seconds since we are forking a command that does crypto potentially at early boot.

Copy link
Contributor

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

Marking approve. I'm happy with anything between 5s and your original if you find this isn't working. I reran the failed travis test to see if that was transient as I can't imagine how this affects those tests.

@stevendanna
Copy link
Contributor

Copy link
Contributor

@markan markan left a comment

Choose a reason for hiding this comment

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

Speculating; but we have seen some platforms in FIPS mode take a long time to generate keys, so perhaps that's where the 20 seconds comes from.

I think 5 seconds is just fine for non-fips installs with decent randomness sources.

@jeremymv2 jeremymv2 merged commit 732ec7e into chef:master Oct 4, 2018
@jeremymv2 jeremymv2 deleted the jeremymv2/chef_authn_hab_pkg_defaults branch October 4, 2018 18:06
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.

3 participants