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 test for request conflict with SSNv2 #4898

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

edewata
Copy link
Contributor

@edewata edewata commented Nov 7, 2024

The test for request conflict with SSNv2 has been updated to create a duplicate request with the proper requestId attribute. With this change the CLI is no longer failing, but it still does not create a new request as expected.

EOF

diff expected stderr
--output-file testuser.crt

Copy link
Member

Choose a reason for hiding this comment

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

A request can be used to generate multiple certificates. If a request is provided and it match with the identified record, is there a verification? I mean in this case the csr is the same to the one recorded, it could be OK but If the csr does not match, which one is used? Is there a conflict?
I think this command should use a different CSR, and verify if it works as expected, which for me it should generate an error but other results could be valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While a CSR can be reused for multiple enrollments, a request record in DS is meant to store the info associated to a single enrollment, e.g. the CSR submitted for that enrollment, the enrollment status, the enrollment completion date. It also serves as historical records. We've also been using the same CSR in the existing SSN tests and the CA always creates a new request record for each enrollment.

Please see the updated PR. I've updated the PR to use a new CSR to test the conflict, then check the request record after the enrollment, and it shows that the request record has changed. I think this is definitely a bug because now we've lost the original info in it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, overwrite the request is a bug. I am wondering if this behaviour is the same for RSNv3. If the request exist it gets overwritten. It should be a very rare situation but to investigate. I am expecting the same behaviour for requests and certs. Fortunately, the certs do not get overwritten.
The fix to this is related to the handling of the error in case of range overlap since there should be no other situations where this condition should occur. Maybe we could open an issue just to remember the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think it will affect RSNv3 and also KRA too. I opened ticket #4899 for this issue.

The test for request ID conflict with SSNv2 has been updated
to create a duplicate request record with the proper requestId
attribute. With this change the CLI is no longer failing, but
the CA is still updating the duplicate request record instead
of creating a new one as expected.
Copy link
Member

@fmarco76 fmarco76 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

sonarcloud bot commented Nov 8, 2024

@edewata
Copy link
Contributor Author

edewata commented Nov 8, 2024

@fmarco76 Thanks!

@edewata edewata merged commit 1e824d9 into dogtagpki:master Nov 8, 2024
159 of 168 checks passed
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