Skip to content

Comments

Update some FIPS module things#11177

Closed
richsalz wants to merge 2 commits intoopenssl:masterfrom
richsalz:misc-fips-nits
Closed

Update some FIPS module things#11177
richsalz wants to merge 2 commits intoopenssl:masterfrom
richsalz:misc-fips-nits

Conversation

@richsalz
Copy link
Contributor

@richsalz richsalz commented Feb 25, 2020

  • The generated config for the module signature is "fipsmodule.conf",
    since it's about the module. This is a more sensible name for the user community.
  • Add -q option to fipsinstall command, to stop chatty verbose status
    messages. This is normally done at install/boot time, and we don't want extra status/noise messages.
  • Document env var OPENSSL_CONF_INCLUDE. This is just a doc bug.

@richsalz
Copy link
Contributor Author

richsalz commented Feb 25, 2020

ping @slontis for feedback.

@mspncp
Copy link
Contributor

mspncp commented Feb 25, 2020

Similar as in #11176 one can ask: why is the OpenSSL configuration file called "openssl.cnf", but the fips configuration file "fipsmodule.conf"? Why not use the same file extension throughout?

@slontis
Copy link
Member

slontis commented Feb 25, 2020

Looks ok to me..
I would argue that the -q option may not be a thing you want to turn off at installation time.
If paths are incorrect or the integrity of the module has been modified then it may be beneficial to know the reason for the failure. It should be noted however that it is highly unlikely that the actual self test KATS should ever fail.

@richsalz
Copy link
Contributor Author

richsalz commented Feb 25, 2020

There's a separate PR to rename *.conf to *.cnf (#11176 )

The output suppressed by -q is nothing useful, it just says pass/verified/failure. When installing the same module on a few hundred thousand machines it's not needed.

levitte
levitte reviewed Feb 25, 2020
View reviewed changes
@richsalz
Copy link
Contributor Author

richsalz commented Mar 8, 2020

Rebased; only change was to propagate .conf->.cnf changes.

@richsalz richsalz mentioned this pull request Mar 20, 2020
Closed
@richsalz richsalz mentioned this pull request Mar 28, 2020
Closed
2 tasks
@richsalz
Copy link
Contributor Author

richsalz commented Mar 31, 2020

ping for another review.

@t8m
Copy link
Member

t8m commented Mar 31, 2020

The commit message seems a little bit obsolete, can you please amend it to reflect the actual patch?

@richsalz
Copy link
Contributor Author

richsalz commented Mar 31, 2020

Reworded the commit message, did I get your concern addressed?

@t8m
Copy link
Member

t8m commented Apr 1, 2020

Reworded the commit message, did I get your concern addressed?

Unfortunately no. I do not see any added documentation of OPENSSL_CONF_INCLUDE env var. And the configuration file is named fipsmodule.cnf not .conf.

t8m
t8m approved these changes Apr 1, 2020
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Approving anyway. The commit message can be amended before merge.

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: otc review pending labels Apr 1, 2020
@richsalz
Copy link
Contributor Author

richsalz commented Apr 1, 2020

I'm not going to touch this to avoid resetting the timers, and I assume you'll fix the commit message, @t8m

@t8m t8m self-assigned this Apr 1, 2020
@openssl-machine
Copy link
Collaborator

openssl-machine commented Apr 2, 2020

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@t8m t8m added approval: review pending This pull request needs review by a committer and removed approval: done This pull request has the required number of approvals labels Apr 2, 2020
@t8m
Copy link
Member

t8m commented Apr 2, 2020

@slontis I've forgot to ask for your reapproval. Does your approval still hold?

richsalz added 2 commits April 21, 2020 13:54
@richsalz
Update some nits around the FIPS module
d3028d0 
- Changed the generated FIPS signature file to be "fipsmodule.conf"
since it contains information about the FIPS module/file.
- Add -q option to fipsinstall command, to stop chatty verbose status
messages.
- Document env var OPENSSL_CONF_INCLUDE
@richsalz
Rename fipsinstall.cnf->fipsmodule.cnf
7a6eefc 
Introduced by rebasing
@richsalz
Copy link
Contributor Author

richsalz commented Apr 21, 2020

I rebased this, to capture some additional fipsinstall.cnf -> fipsmodule.cnf renamings that had to be done.

Fresh approvals needed. :(

t8m
t8m approved these changes Apr 22, 2020
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Still OK. @slontis can you please re-approve?

slontis
slontis approved these changes Apr 22, 2020
View reviewed changes
Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

Reapproved

@t8m t8m removed the approval: review pending This pull request needs review by a committer label Apr 22, 2020
@t8m t8m added the approval: done This pull request has the required number of approvals label Apr 22, 2020
@openssl-machine
Copy link
Collaborator

openssl-machine commented Apr 23, 2020

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

@t8m t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Apr 23, 2020
openssl-machine pushed a commit that referenced this pull request Apr 24, 2020
@richsalz @t8m
Update some nits around the FIPS module
1cd2c1f 
- Changed the generated FIPS signature file to be "fipsmodule.conf"
since it contains information about the FIPS module/file.
- Add -q option to fipsinstall command, to stop chatty verbose status
messages.
- Document env var OPENSSL_CONF_INCLUDE

Reviewed-by: Shane Lontis <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #11177)
openssl-machine pushed a commit that referenced this pull request Apr 24, 2020
@richsalz @t8m
Rename fipsinstall.cnf->fipsmodule.cnf
713fc10 
Introduced by rebasing

Reviewed-by: Shane Lontis <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #11177)
@t8m
Copy link
Member

t8m commented Apr 24, 2020

Merged to master. Thank you.

@t8m t8m closed this Apr 24, 2020
@richsalz richsalz deleted the misc-fips-nits branch April 24, 2020 14:06
@richsalz richsalz mentioned this pull request Jun 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@levitte levitte levitte left review comments

@t8m t8m t8m approved these changes

@slontis slontis slontis approved these changes

Assignees

@t8m t8m

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@richsalz @mspncp @slontis @t8m @openssl-machine @levitte