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

Explicit SSNv2 hex input #4893

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Conversation

fmarco76
Copy link
Member

@fmarco76 fmarco76 commented Nov 4, 2024

When legacy2 generator is in use the hex input parameter must have the 0x... format. If decimal value are provided the pkispawn fails to make aware the user on what value are used.

@fmarco76 fmarco76 requested a review from edewata November 4, 2024 19:46
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.

I have some comments but feel free to update/merge.

Comment on lines +1260 to +1262
if config.str2bool(self.mdict['pki_random_serial_numbers_enable']):
subsystem.set_config('dbs.enableRandomSerialNumbers', 'true')
subsystem.set_config('dbs.randomSerialNumberCounter', '0')
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 thinking to disallow this param for SSNv2 and ask the admin to use RSNv3 instead. We can do that separately later.

Copy link
Member Author

@fmarco76 fmarco76 Nov 5, 2024

Choose a reason for hiding this comment

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

Is there a reason to explicitly avoid this configuration? It should work.

Comment on lines 1361 to 1365
if key_id_generator == 'legacy2':
serial_dn = 'ou=keyRepository,ou=ranges_v2'
else:
serial_dn = 'ou=keyRepository,ou=ranges'
subsystem.set_config('dbs.serialRangeDN', serial_dn)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can simplify this code since the generator can only be legacy2 here. Same thing for the legacy case above.

Comment on lines 1329 to 1331
####################################################################################################
# Try to install with wrong parameter foramt
- name: Create CA with unsupported range format
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: foramt

We probably should run this step before the correct pkispawn invocation. That will simulate the actual scenario that people might encounter, i.e. run pkispawn with the wrong params first, then rerun it with the correct params.

serial_dn = 'ou=keyRepository,ou=ranges'
subsystem.set_config('dbs.serialRangeDN', serial_dn)

else: #random
Copy link
Contributor

Choose a reason for hiding this comment

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

I think pylint requires 2 spaces before the # sign.

Since SSNv2 ranges number requires the format `0x...` to be correctly
interpreted as hex number, an exception is introduced when a decimal
number is provided. This approach make explicit the hex or dec number
are in use and avoid later problems.
@fmarco76 fmarco76 force-pushed the explicitSSNv2HexInput branch from 8b3b298 to 688b516 Compare November 5, 2024 09:57
@fmarco76 fmarco76 force-pushed the explicitSSNv2HexInput branch from 688b516 to 9d8b059 Compare November 5, 2024 10:04
@fmarco76
Copy link
Member Author

fmarco76 commented Nov 5, 2024

@edewata Thanks! I have updated to fix your comments. It remains the doubt if the SSNv2 with random should be supported which I think it is acceptable. If it is not the case we can fix in following PRs.

@fmarco76 fmarco76 merged commit aa99ce6 into dogtagpki:master Nov 5, 2024
157 of 167 checks passed
@fmarco76 fmarco76 deleted the explicitSSNv2HexInput branch November 5, 2024 11:24
Copy link

sonarqubecloud bot commented Nov 5, 2024

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