Skip to content

fix: serial number stored in DB now matches certificate serial number#212

Open
devangpratap wants to merge 1 commit intoopenwisp:masterfrom
devangpratap:master
Open

fix: serial number stored in DB now matches certificate serial number#212
devangpratap wants to merge 1 commit intoopenwisp:masterfrom
devangpratap:master

Conversation

@devangpratap
Copy link

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

Reference to Existing Issue

Closes #121

Description of Changes

_generate() built the certificate using int(self.serial_number) but never read
the serial number back after signing. The DB value came from _generate_serial_number()
while the certificate value came from what the signing library actually embedded. In the
pyOpenSSL path set_serial_number() silently truncated 128-bit UUID integers during
signing, producing two genuinely different integers — exactly what was reported in #121.

Fix: after cert = builder.sign(...), sync the field back:
`self.serial_number = str(cert.serial_number)

Added test_serial_number_db_matches_certificate to verify DB and certificate serial
always match. All 74 existing tests continue to pass.

Screenshot

N/A

@coderabbitai
Copy link

coderabbitai bot commented Mar 1, 2026

Warning

Rate limit exceeded

@devangpratap has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 19 minutes and 44 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 9056c37 and 39ee622.

📒 Files selected for processing (2)
  • django_x509/base/models.py
  • django_x509/tests/test_cert.py
📝 Walkthrough

Walkthrough

The PR fixes a serial number mismatch by assigning the generated certificate's serial number to the model instance (self.serial_number = str(cert.serial_number)) in _generate within django_x509/base/models.py. A test test_serial_number_db_matches_certificate was added to ensure the stored serial_number on the DB model matches the X.509 certificate's serial number when reloaded from the database.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main fix: ensuring the serial number stored in the database matches the certificate's serial number.
Description check ✅ Passed The description covers all required sections from the template: checklist items (with appropriate marks), reference to issue #121, detailed explanation of the changes and root cause, and test information. Only documentation update was intentionally skipped.
Linked Issues check ✅ Passed The pull request directly addresses issue #121 by fixing the serial number mismatch between the database and the certificate through synchronization after signing, and adds a test to verify the fix.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the serial number mismatch issue: modifying _generate() to sync the serial number and adding a test to verify the fix. No out-of-scope changes are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…isp#121

The _generate() method never read the serial number back after signing. In the pyOpenSSL path, set_serial_number() silently truncated 128-bit UUID integers, causing the DB and certificate to store different values. Fixed by syncing the field from the signed cert after builder.sign().

Fixes openwisp#121
@coveralls
Copy link

Coverage Status

coverage: 94.881% (+0.009%) from 94.872%
when pulling 39ee622 on devangpratap:master
into fc872b8 on openwisp:master.

@stktyagi
Copy link
Member

stktyagi commented Mar 3, 2026

Hey @devangpratap, can you reproduce and check the existence of this issue as this was fixed in #194 while migrating from PyOpenSSL to cryptography.

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.

Serial number mismatch?

3 participants