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

Relocate SSNv2 nextRange attribute #4890

Merged
merged 2 commits into from
Nov 4, 2024
Merged

Conversation

edewata
Copy link
Contributor

@edewata edewata commented Nov 1, 2024

The nextRange attribute for SSNv2 has been moved to ou=requests,ou=ranges_v2 for requests and ou=certificateRepository,ou=ranges_v2 for certs such that the nextRange for SSNv1 is unchanged during migration. This will simplify the recovery process in case there's an issue during migration.

The SubsystemIdGeneratorUpdateCLI has been updated to compute the request nextRange for SSNv2 with the same process used to compute the cert nextRange for SSNv2.

The SSNv1 and SSNv2 tests have been updated to use the new SSNv2 nextRange location.

The nextRange attribute for SSNv2 has been moved to
ou=requests,ou=ranges_v2 for requests and
ou=certificateRepository,ou=ranges_v2 for certs such that
the nextRange for SSNv1 is unchanged during migration.
This will simplify the recovery process in case there's
an issue during migration.

The SubsystemIdGeneratorUpdateCLI has been updated to
compute the request nextRange for SSNv2 with the same
process used to compute the cert nextRange for SSNv2.
The SSNv1 and SSNv2 tests have been updated to use the new
SSNv2 nextRange location.
@edewata edewata requested a review from fmarco76 November 1, 2024 23:47
Copy link

sonarqubecloud bot commented Nov 2, 2024

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

LDAPModification mods = new LDAPModification(LDAPModification.REPLACE, attrRequestNextRange);

conn.modify(newRangeDN, mods);
}
Copy link
Member

Choose a reason for hiding this comment

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

Actually, these check should not be needed since there are not gap/conversion for the request numbers so the nextRange could be just copied to the new location. However, it should not add error and it could be a double check for the code which is almost the same of the serial number. Maybe they could be merged but it is OK to leave as it is for the moment

@edewata
Copy link
Contributor Author

edewata commented Nov 4, 2024

@fmarco76 Thanks! I'll merge.

Yes, ideally they should be merged so we don't have to maintain the code twice. One thing though, I was expecting the SSNv2 nextRange calculated by this code to be identical to the SSNv1 nextRange since there is no gap, but they are in fact different. Hopefully that's not an issue.

@edewata edewata merged commit 678b8d0 into dogtagpki:master Nov 4, 2024
158 of 168 checks passed
@fmarco76
Copy link
Member

fmarco76 commented Nov 4, 2024

One thing though, I was expecting the SSNv2 nextRange calculated by this code to be identical to the SSNv1 nextRange since there is no gap, but they are in fact different. Hopefully that's not an issue.

In which case are they different? Looking at the tests I do not see differences!

@edewata
Copy link
Contributor Author

edewata commented Nov 4, 2024

In the single CA case the next range changes from 41 to 51 as soon as we switch to SSNv2 (no new enrollment):
https://github.com/dogtagpki/pki/blob/master/.github/workflows/ca-ssnv1-test.yml#L1301-L1378

In CA clone case it changes from 31 to 51:
https://github.com/dogtagpki/pki/blob/master/.github/workflows/ca-clone-ssnv1-test.yml#L1300-L1370

@fmarco76
Copy link
Member

fmarco76 commented Nov 4, 2024

I thought this increment was because of the restart and the allocation of a new range. The range was exhausted when we switch to legacy2.

@edewata
Copy link
Contributor Author

edewata commented Nov 4, 2024

In SSNv1 we've observed the range changing when it's exhausted during enrollment. We don't have a test to see if the range changes when the server is restarted, so I'm not sure if this is the proper behavior. Ideally I don't think it should, but so far it doesn't seem to be causing any problem.

@fmarco76
Copy link
Member

fmarco76 commented Nov 4, 2024

In SSNv1 we have 40 requests on line 1105. The nextRange is 41 on line 1165 and on line 1237 there is the generator switch to legacy2. There are no range allocation before the switch. During the start there is a range allocation so the nextRange go to 51. If we restart without switch the new range is allocated and the nextRange go to 51 so there is no problem (actually, before to see your test on range I was restarting the server to generate the new range instead of running the job).

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