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

Fix ops flag mask master #4649

Merged
merged 4 commits into from
Jan 8, 2024
Merged

Conversation

fmarco76
Copy link
Member

@edewata this PR add two flags pki_*_opsFlag and pki_*_opsFlagMask. These have been introduced in branch v10.13 with PRs #4643, #4645 and #4647 but the code in master is too different for cherry-picking so I have written from scratch and added test and doc.

Default operation flags does not work for some HSM so a new mechanism is
introduced to allow a custom flag list. The certificate generate in the
hsm can be customised in pkispawn config file with `pki_<cert>_opsFlagMask` where `<cert> is one between:

- audit_signing
- sslserver
- subsystem
- ca_signing
- ocsp_signing
- storage
- transport
- ocsp_signing

The value is a comma sepated list of flags (case insensitive) which can
be: encrypt, decrypt, sign, sign_recover, verify, verify_recover, wrap,
unwrap AND derive.

The same flags can be used with the command `pki nss-key-create` where
the new flag `--ops-flag-mask` is added. The value is the a csv as
above.
Add the second parameter to pkispawn in other to customise the key par generation in HSM. The new config parameter is `pki_<cert>_opsFlag`. The `<cert>` is the id as defined for the parameter `pki_<cert>_opsFlagMask` and they have the same values
@fmarco76 fmarco76 requested a review from edewata December 21, 2023 23:51
Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

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

Feel free to merge since the functionality has been added into other branches, but please see my comments & questions below. We can address them in separate PRs if needed.

@@ -86,6 +86,14 @@ public void createOptions() {
option.setArgName("boolean");
options.addOption(option);

option = new Option(null, "ops-flag", true, "Custom flags for key usage (empty for HSM default)");
Copy link
Contributor

Choose a reason for hiding this comment

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

Just thinking, would --op-flags / op-flags-mask be more appropriate since these options contains a list of flags?

It's probably not necessary to mention (empty for HSM default) since the list will be empty by default for all types of tokens, not just HSM.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about the wording but your clarification seems reasonable so I'll update.

Comment on lines 111 to 112
-D pki_ca_signing_opsFlag=sign \
-D pki_ca_signing_opsFlagMask=sign \
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too familiar with how the flag & mask work in NSS, but does this mean the mask should ultimately disable the sign operation in NSS so the installation should fail too? Or does opsFlagMask simply remove the values from opsFlag (in that case the mask might not be that useful since the user can just specify the correct opsFlag without the masked value in the first place)?

Copy link
Member Author

Choose a reason for hiding this comment

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

opsFlagMask is applied to the flags identified by NSS, then the opsFlag are added to the remaining operation. This test show that the opsFlag are added to the resulting list. (I will change all opsFlag* is opFlags*)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, does it mean the opsFlag param overrides opsFlagMask? Does it work that way too with --keyOpFlagsOn and --keyOpFlagsOff in certutil? We probably should document this behavior.

Comment on lines 63 to 64
and their value is a comma separated list of the following flags: encrypt, decrypt, sign,
sign_recover, verify, verify_recover, wrap, unwrap and derive. The first parameter add flags to
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest adding back quotes for the actual values that can be specified in these params, e.g. encrypt, decrypt.

The option are renamed to be more coherent with their meaning as:
- op-flags
- op-flags-mask
@fmarco76 fmarco76 force-pushed the FixOpsFlagMask_master branch from 35fe02f to 0d4c905 Compare January 8, 2024 16:09
Copy link

sonarqubecloud bot commented Jan 8, 2024

@fmarco76 fmarco76 merged commit 185a865 into dogtagpki:master Jan 8, 2024
131 checks passed
@fmarco76
Copy link
Member Author

fmarco76 commented Jan 8, 2024

@edewata Thanks! I have updated the code following your comments.

@fmarco76 fmarco76 deleted the FixOpsFlagMask_master branch January 8, 2024 17:57
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