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.

Sign up for GitHub

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

Jump to bottom

Fix test for request conflict with SSNv2 #4898

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Fix test for request conflict with SSNv2 #4898

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 48 additions & 26 deletions .github/workflows/ca-ssnv2-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1255,22 +1255,24 @@ jobs:
diff expected output

####################################################################################################
# Enroll a cert with a conflicting request ID
# Enroll a cert with a conflicting request record ID
#
# This simulates a scenario where there is already a completed request in
# the database, possibly due to a bug or an incorrect range configuration,
# with an ID that will be used by the next request created by the CA.
# This simulates a scenario where there is already a completed request
# record in the database, possibly due to a bug or an incorrect range
# configuration, with an ID that will be used by the next request record
# created by the CA.
#
# Ideally the conflict should be handled transparently, so a new request
# should be created with a new ID, leaving the conflicting request intact,
# and a new cert should be issued as usual.
# record should be created with a new ID, leaving the conflicting request
# record intact, and a new cert record should be issued as usual.
#
# However, currently there is no new request created, the conflicting
# request is changed to pending, and the CLI is failing.
# However, currently the CA reuses and modifies the conflicting request
# record and there is no new request record created.

- name: Create a request with the next ID
- name: Create a request record with the next ID
if: always()
run: |
# get the latest request record
docker exec ds ldapsearch \
-H ldap://ds.example.com:3389 \
-D "cn=Directory Manager" \
Expand All @@ -1281,57 +1283,77 @@ jobs:
-o ldif_wrap=no \
-LLL | tee request.ldif

# replace the ID with the next ID
sed -i \
-e "s/^dn: cn=37,/dn: cn=38,/" \
-e "s/^serialno: 0237/serialno: 0238/" \
-e "s/^requestId: 0237/requestId: 0238/" \
-e "s/^extdata-requestid: 37/extdata-requestid: 38/" \
-e "s/^cn: 37/cn: 38/" \
request.ldif

# add the updated request record
docker exec ds ldapadd \
-H ldap://ds.example.com:3389 \
-D "cn=Directory Manager" \
-w Secret.123 \
-x \
-f $SHARED/request.ldif

- name: Enroll a cert with a conflicting request ID
- name: Enroll a cert with a conflicting request record ID
if: always()
run: |
# create a new CSR
docker exec pki pki \
nss-cert-request \
--subject "uid=testuser2" \
--ext /usr/share/pki/tools/examples/certs/testuser.conf \
--csr testuser2.csr

# the CLI should complete successfully
docker exec pki pki \
-n caadmin \
ca-cert-issue \
--profile caUserCert \
--csr-file testuser.csr \
--output-file testuser.crt \
> >(tee stdout) 2> >(tee stderr >&2) || true
--csr-file testuser2.csr \
--output-file testuser2.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 CLI should complete successfully, but currently it's failing
cat > expected << EOF
ERROR: Request pending
EOF

diff expected stderr

- name: Check requests
- name: Check request records
if: always()
run: |
docker exec pki pki-server ca-cert-request-find | tee output
sed -n "s/^ *Request ID: *\(.*\)$/\1/p" output > actual

# there should be 39 requests (37 existing + 1 conflicting + 1 new)
# but currently the CA reuses the conflicting request instead of
# there should be 39 request records (37 existing + 1 conflicting + 1 new)
# but currently the CA reuses the conflicting request record instead of
# creating a new one
seq 1 38 > expected

diff expected actual

- name: Check certs
- name: Check conflicting request record
if: always()
run: |
docker exec ds ldapsearch \
-H ldap://ds.example.com:3389 \
-D "cn=Directory Manager" \
-w Secret.123 \
-x \
-b "cn=38,ou=ca,ou=requests,dc=ca,dc=pki,dc=example,dc=com" \
-s base \
-o ldif_wrap=no \
-LLL | tee request-after.ldif

# the conflicting request record should not change
# but currently it is updated to store the new CSR
diff request.ldif request-after.ldif || true

- name: Check cert records
if: always()
run: |
docker exec pki pki-server ca-cert-find | tee output
sed -n "s/^ *Serial Number: *\(.*\)$/\1/p" output > actual

# there should be 37 certs (36 existing + 1 new)
# there should be 37 cert records (36 existing + 1 new)
printf "0x%x\n" {9..45} > expected

diff expected actual
Expand Down
Loading