Issue 6951 - Dynamic Certificate refresh phase 4 - Update lib389 and dsconf #7171
Conversation
…dsconf Desciption: Add the CertManager abstraction layer and DynamicCerts backend module. Enhance the NssSsl backend to support importing PKCS#12 containers, containing cert and private key. Update dsconf to support new PKCS#12-related args, allowing users to supply passwords via text, stdin, or file. Fix: - Introduce CertManager abstraction layer for uniform cert management. - Implement DynamicCerts backend with add/list/delete operations. - Extend NssSsl.add_cert to handle PKCS#12 files with passwords via text, stdin, or file. - Update dsconf CLI to accept PKCS#12 arguments (--pkcs12-pin-text, --pkcs12-pin-stdin, --pkcs12-pin-path). - Add tests for PEM, DER and PKCS#12 certificates covering both backends. Relates: 389ds#6951 Reviewed by:
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- The new
RSACertificatehelper inlib389/utils.pypulls in substantial crypto functionality that appears to be used only by tests; consider moving it into a test-specific module (or a dedicated test helper) instead of a general-purpose utils module to keep the public API and runtime dependencies lean. - For the new PKCS#12 password arguments in
security.cert_add(--pkcs12-pin-text,--pkcs12-pin-stdin,--pkcs12-pin-path), it would be safer to place them in a mutually exclusive argument group and clarify their help texts (e.g. explicitly noting stdin/file behavior) to avoid ambiguous combinations and improve CLI UX. - The newly added
--do-itflag oncerts addis parsed but not used incert_add; either wire it into the actual verification/override behavior or drop it to avoid exposing a no-op option.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `RSACertificate` helper in `lib389/utils.py` pulls in substantial crypto functionality that appears to be used only by tests; consider moving it into a test-specific module (or a dedicated test helper) instead of a general-purpose utils module to keep the public API and runtime dependencies lean.
- For the new PKCS#12 password arguments in `security.cert_add` (`--pkcs12-pin-text`, `--pkcs12-pin-stdin`, `--pkcs12-pin-path`), it would be safer to place them in a mutually exclusive argument group and clarify their help texts (e.g. explicitly noting stdin/file behavior) to avoid ambiguous combinations and improve CLI UX.
- The newly added `--do-it` flag on `certs add` is parsed but not used in `cert_add`; either wire it into the actual verification/override behavior or drop it to avoid exposing a no-op option.
## Individual Comments
### Comment 1
<location> `src/lib389/lib389/dyncerts.py:46-55` </location>
<code_context>
+ self.dirsrv = dirsrv
+ self.conn = None
+
+ def _ensure_ldapi_connection(self):
+ """
+ Ensure LDAP connection is initialized and bound using SASL EXTERNAL.
+
+ Returns:
+ None
+ """
+ if self.conn is not None:
+ return
+
+ try:
+ ldapiurl, _ = get_ldapurl_from_serverid(self.dirsrv.serverid)
+ self.conn = ldap.initialize(ldapiurl)
+ self.conn.protocol_version = 3
+ self.conn.sasl_interactive_bind_s("", ldap.sasl.external())
+ log.debug(f"DynamicCert connected via LDAPI: {ldapiurl}")
+ except ldap.LDAPError as e:
+ self.conn = None
+ raise RuntimeError(f"Failed to connect via LDAPI: ({ldapiurl}: {e})"
+ )
+
</code_context>
<issue_to_address>
**issue (bug_risk):** DynamicCerts.is_online does not handle RuntimeError from _ensure_ldapi_connection, preventing NSS fallback in CertManager.
Because is_online only catches ldap.LDAPError/NO_SUCH_OBJECT, the RuntimeError raised here will escape, so CertManager won’t fall back to NssSsl when LDAPI connection fails (e.g. bad URL, permissions). Please either have _ensure_ldapi_connection raise an appropriate ldap.LDAPError subclass, or broaden is_online’s exception handling (e.g. catch Exception and return False) so connection failures correctly trigger the NSS fallback.
</issue_to_address>
### Comment 2
<location> `src/lib389/lib389/dyncerts.py:255-124` </location>
<code_context>
+ db_path = inst.get_cert_dir()
+ pw_file = os.path.join(db_path, "pwdfile.txt")
+
+ if is_ca:
+ if pem is None:
+ raise ValueError("PEM file must be provided for CA certificate installation")
+ run([
+ 'certutil',
+ '-A',
+ '-n', nickname,
+ '-t', 'CT,,',
+ '-f', pw_file,
+ '-d', db_path,
+ '-a',
+ '-i', pem
+ ], check=True)
+ else:
+ if p12 is None or p12_password is None:
+ raise ValueError("PKCS#12 file and password must be provided for server certificate installation")
</code_context>
<issue_to_address>
**issue (bug_risk):** Deleting trust flags for non-CA certs may fail if the attribute is absent or has a different value.
For updates to existing non-CA entries, `MOD_DELETE` is used with a specific value `b",,"`, which will only work if the attribute is present and exactly matches that value. If the trust attribute is missing, has a different value, or was already cleared, this will likely raise an LDAP error (e.g. `NO_SUCH_ATTRIBUTE`). To avoid this, delete the attribute without specifying a value (i.e. `MOD_DELETE` with `None`), or conditionally add the delete operation only when the attribute is present.
</issue_to_address>
### Comment 3
<location> `src/lib389/lib389/nss_ssl.py:1206-1215` </location>
<code_context>
+ args.primary_cert = False
+
+ stdin_backup = sys.stdin
+ try:
+ sys.stdin = io.StringIO(server_cert.P12_PASSWORD + "\n")
+ cert_add(inst, DEFAULT_SUFFIX, lc.log, args)
</code_context>
<issue_to_address>
**suggestion:** The new error handling for certutil calls drops the stderr output, making failures harder to diagnose.
The old `CalledProcessError` path exposed `e.output.decode('utf-8').rstrip()`, which usually contains certutil’s detailed error text. The new `ValueError(f"Failed to add certificate: {nickname} {e}")` generally only shows the return code and command. To retain useful diagnostics, please include stderr when present (e.g., via `getattr(e, "output", None)` and appending a decoded version to the `ValueError` message).
</issue_to_address>
### Comment 4
<location> `src/lib389/lib389/cert_manager.py:84-93` </location>
<code_context>
+ """
+ return self.backend.list_certs()
+
+ def get_cert_details(self, nickname: str):
+ """
+ Get certificate details by nickname/CN.
+
+ Returns:
+ dict containing certificate details (CN, subject, issuer, notBefore, notAfter, trustFlags)
+ None if certificate does not exist
+ """
+ try:
+ return self.backend.get_cert_details(nickname=nickname)
+ except ValueError as e:
+ if "not found" in str(e).lower():
+ return None
+ raise
</code_context>
<issue_to_address>
**suggestion (bug_risk):** CertManager.get_cert_details relies on backend error message text containing 'not found', which is brittle.
Mapping backend errors to `None` by checking for `'not found'` in the `ValueError` message couples behavior to backend error wording. If those messages change, real errors could be misclassified as "not found" or vice versa. Consider having backends raise a dedicated exception (e.g. `CertNotFound`) or return `None` explicitly when a certificate is missing, so `CertManager` doesn’t need to parse exception strings.
Suggested implementation:
```python
def get_cert_details(self, nickname: str):
"""
Get certificate details by nickname/CN.
Returns:
dict containing certificate details (CN, subject, issuer, notBefore, notAfter, trustFlags)
None if certificate does not exist
Raises:
CertNotFound: if the certificate does not exist and the backend uses exceptions
"""
try:
return self.backend.get_cert_details(nickname=nickname)
except CertNotFound:
# Normalize "certificate missing" to None for callers that expect a sentinel
return None
```
To fully implement the suggestion and remove reliance on parsing error message text:
1. Ensure a `CertNotFound` exception exists and is imported/available in this module. For example, near the top of `cert_manager.py` you can either:
- Import from a shared exceptions module (preferred if it exists):
`from lib389.exceptions import CertNotFound`
- Or define a local exception:
`class CertNotFound(Exception): """Raised when a certificate is not found in the backend."""`
2. Update all certificate backend implementations used by `CertManager` so that `get_cert_details(nickname=...)`:
- Raises `CertNotFound` when a certificate is missing (instead of `ValueError` with a "not found" message), or
- Returns `None` directly for missing certificates, in which case they will pass through unchanged and the `CertNotFound` handler here will simply never trigger.
3. If there are existing callers that specifically expect a `ValueError` for missing certs, update them to either:
- Expect `None` from `CertManager.get_cert_details`, or
- Catch `CertNotFound` if they call the backend directly.
</issue_to_address>
### Comment 5
<location> `src/lib389/lib389/cli_conf/security.py:551-552` </location>
<code_context>
cert_add_parser = certs_sub.add_parser('add', help='Add a server certificate', description=(
'Add a server certificate to the NSS database'))
cert_add_parser.add_argument('--file', required=True,
- help='Sets the file name of the certificate')
+ help='Sets the file pkcs12 name of the certificate')
+ cert_add_parser.add_argument('--pkcs12-pin-text', required=False,
+ help='The pkcs12 password')
</code_context>
<issue_to_address>
**nitpick:** The help text for --file suggests only PKCS#12 is accepted, but the code still supports PEM/DER.
Since cert_add still accepts PEM/DER as well as PKCS#12 (.p12/.pfx), the new help text ('file pkcs12 name of the certificate') is misleading. Please update it to reflect all supported formats, e.g. 'Sets the certificate file (PEM, DER, or PKCS#12)'.
</issue_to_address>
### Comment 6
<location> `src/lib389/lib389/cli_conf/security.py:248-251` </location>
<code_context>
- # Add the cert
- tlsdb.add_cert(args.name, args.file)
+ cert_exists = certmgr.get_cert_details(args.name)
+ if cert_exists:
+ log.info(f"Certificate '{args.name}' already exists, skipping")
+ return
- log.info("Successfully added certificate")
</code_context>
<issue_to_address>
**question (bug_risk):** Changing cert_add to silently skip existing certificates alters previous behavior and may hide configuration mistakes.
Previously, a duplicate nickname caused a ValueError; now any truthy certmgr.get_cert_details result leads to an early return with only an info log. This can break callers/scripts that rely on a hard failure for duplicates. If this change is intentional, please ensure the CLI/docs and upstream callers reflect the new semantics; otherwise, consider restoring the explicit error or making “skip if exists” opt-in via a flag.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| try: | ||
| check_output(cmd, stderr=subprocess.STDOUT) | ||
| except subprocess.CalledProcessError as e: | ||
| raise ValueError(f"Failed to import PKCS#12 {input_file} {e}") | ||
|
|
||
| if ca: | ||
| cmd = [ | ||
| "certutil", | ||
| "-M", | ||
| "-n", nickname, |
There was a problem hiding this comment.
suggestion: The new error handling for certutil calls drops the stderr output, making failures harder to diagnose.
The old CalledProcessError path exposed e.output.decode('utf-8').rstrip(), which usually contains certutil’s detailed error text. The new ValueError(f"Failed to add certificate: {nickname} {e}") generally only shows the return code and command. To retain useful diagnostics, please include stderr when present (e.g., via getattr(e, "output", None) and appending a decoded version to the ValueError message).
progier389
left a comment
progier389
left a comment
There was a problem hiding this comment.
Some changes are needed around CertManager and Dyncerts
src/lib389/lib389/cert_manager.py
Outdated
| otherwise falls back to NSS DB. | ||
| """ | ||
|
|
||
| def __init__(self, dirsrv, backend=None): |
There was a problem hiding this comment.
backend is confusing should maybe use another term like: handler
And IMHO it should not be a parameter but directly initialized in init:
self.handler = DynamicCerts(dirsrv) if dirsrv.status() else NssSsl(dirsrv)
src/lib389/lib389/cert_manager.py
Outdated
| """ | ||
| try: | ||
| return self.backend.get_cert_details(nickname=nickname) | ||
| except ValueError as e: |
There was a problem hiding this comment.
IMHO sourcery is right.
ValueError should not be handled at this level but directly at the handler level
src/lib389/lib389/cert_manager.py
Outdated
| if not nickname: | ||
| raise ValueError("Certificate nickname must not be empty") | ||
|
|
||
| if isinstance(self.backend, DynamicCerts): |
There was a problem hiding this comment.
IMHO you should not test the instance but rather have a sub function both in NssSsl and DynamicCerts in which you put the dedicated code
src/lib389/lib389/dyncerts.py
Outdated
| DYCATTR_NOTAFTER = DYCATTR_PREFIX + "notafter" | ||
| DYCATTR_FORCE = DYCATTR_PREFIX + "Force" | ||
|
|
||
| class DynamicCerts: |
There was a problem hiding this comment.
I wonder if we should not rather have:
class DynamicCert(DSLdapObject) :
and class DynamicCerts(DSLdapObjects) :
to reuse the standard entry framework
to list/retrieve a certificate (acceding attribute with entry.get_attr_val_utf8(attr) )
And extend them if needed.
There was a problem hiding this comment.
I agree with you, let me look into this
| cert_add_parser.add_argument('--pkcs12-pin-text', required=False, | ||
| help='The pkcs12 password') | ||
| cert_add_parser.add_argument('--pkcs12-pin-stdin', required=False, | ||
| help='The pkcs12 password') |
There was a problem hiding this comment.
Can you elaborate on the help text here? They probably shouldn't both be the same :-)
|
Ok, so I was having issues with CI tests so reverted them for now, I validate with dsconf for all cert operations. I will add CI tests soon, but as other work is dependant on this work I pushed the non CI test changes. |
src/lib389/lib389/cert_manager.py
Outdated
| :param instance: DirSrv instance | ||
| :raises ValueError: If instance is None | ||
| """ | ||
| if instance is None: |
There was a problem hiding this comment.
should better be:
if not isinstance(instance, DirSrv):
src/lib389/lib389/dyncerts.py
Outdated
| :return: True if certificate is a CA, else False | ||
| """ | ||
| # Check trust flags first | ||
| trust_flags = self.get_attr_val_utf8(DYCATTR_TRUST) |
There was a problem hiding this comment.
not sure that trust flag are the most reliable to check for CA.
(maybe using the cert_is_ca function in util.py)
There was a problem hiding this comment.
Yes I agree, its not ideal. I added DER blob support to cert_is_ca(), which is fine when we are adding a cert as we have the cert file in the NSS case or the cert DER blob in the dyncert case. But when we list certs we dont, we just get a list of child entries, no cert file or cert blob. This is the reason we use a certs trust flags, like NssSsl.
There was a problem hiding this comment.
It is relatively easy to get the der:
offline: certutil -L -d certdir -n nickname -r
online: get dsDynamicCertificateDer value from the object in the list
maybe we should have a cert_and_der_list that return a list of couples (nickname, der)
There was a problem hiding this comment.
Hmm, when i try access the cert object during listing, I get
INFO: JC list_certs cert cn=SERVER_CERT_PEM,cn=dynamiccertificates der length: 0
[snip]
cert_objects = self.list()
certs = []
for cert in cert_objects:
#JC test get cert der
if cert._dn != self._basedn:
der_list = cert.get_attr_vals_bytes(DYCATTR_CERTDER)
der_bytes = der_list[0] if der_list else None
log.info(f"JC list_certs cert:{cert._dn} der length: {len(der_bytes) if der_bytes else 0}")
[19/Jan/2026:13:31:11.634987751 +0000] conn=115 op=5 SRCH base="cn=SERVER_CERT_PEM,cn=dynamiccertificates" scope=0 filter="(objectClass=*)" attrs="dsdynamiccertificateder"
[19/Jan/2026:13:31:11.643515349 +0000] conn=115 op=5 RESULT err=0 tag=101 nentries=1 wtime=0.000175585 optime=0.008529738 etime=0.008704572
[snip]
It looks like i am adding the cert correctly:
[19/Jan/2026:13:31:11.617178499 +0000] conn=115 op=4 ADD dn="cn=SERVER_CERT_PEM,cn=dynamiccertificates"
[19/Jan/2026:13:31:11.634527343 +0000] conn=115 op=4 RESULT err=0 tag=105 nentries=0 wtime=0.000166210 optime=0.017355309 etime=0.017520674
dn = f"cn={nickname},{self._basedn}"
attrs = {
"cn": [nickname.encode()],
"objectClass": [b"top", b"extensibleObject"],
DYCATTR_CERTDER: [ der_cert, ]
}
[snip]
cert_obj = self.get_cert_obj(nickname)
if not cert_obj:
cert_obj = DynamicCert(self._instance, dn)
cert_obj.create(properties=attrs)
else:
attrs_list = [(attr, vals) for attr, vals in attrs.items()]
cert_obj.replace_many(*attrs_list)
There was a problem hiding this comment.
You are right, there is bug in dyncert code:
ldapsearch -Q -LLL -Y EXTERNAL -H ldapi://%2fhome%2fprogier%2fsb%2f389%2ftst%2fci-install%2fvar%2frun%2fslapd-supplier1.socket -b cn=dynamiccertificates dsdynamiccertificateder
dn: cn=dynamiccertificates
dn: cn=Self-Signed-CA,cn=dynamiccertificates
dsdynamiccertificateder:
dn: cn=Server-Cert,cn=dynamiccertificates
dsdynamiccertificateder:
Will open an issue to fix it.
There was a problem hiding this comment.
Ok, I will make the suggested changes here.
There was a problem hiding this comment.
That bug exposed a bug I had in cert_is_ca, working now
blob support to cert_is_ca
|
|
||
| return cert | ||
|
|
||
| def get_cert_details(self, nickname: str): |
There was a problem hiding this comment.
Not all get_cert_details() usages are fixes. For example _lint_certificate_expiration will break.
There was a problem hiding this comment.
I missed that one, thanks
| raise ValueError("Certificate '{}' not found in NSS database".format(nickname)) | ||
| return None | ||
|
|
||
| def list_certs(self, ca=False): |
There was a problem hiding this comment.
IIUC, security_enable() will break because it uses old format - RSA(inst).set('nsSSLPersonalitySSL', certs[0][0])
| raise ValueError("PKCS#12 CA certificates not supported. Use PEM or DER file") | ||
|
|
||
| certmgr = CertManager(instance=inst) | ||
| certmgr.add_ca_cert(args.file, args.name, args.force) |
There was a problem hiding this comment.
Do we still support bundles? I think this function doesn't unless I miss something
There was a problem hiding this comment.
We decided not to support CA cert bundles for dyncerts, but, it is something we might add in the future.
| return | ||
|
|
||
| for cert in certs: | ||
| _dump_cert(cert, json_output=args.json) |
There was a problem hiding this comment.
I wonder is UI will function correctly here (or if we have a regression) -- for cert_list/cacert_list -- _dump_cert prints a single "certificate" object and is called once per cert. With --json, this yields multiple top-level JSON objects, not a single JSON list as before.
There was a problem hiding this comment.
You are right, the UI expects a list of cert objects, fixed
src/lib389/lib389/utils.py
Outdated
| # p12 | ||
| if cert_file and cert_file.lower().endswith((".p12", ".pfx")): | ||
| if pkcs12_password is None: | ||
| raise ValueError("Password required for PKCS#12 file") |
There was a problem hiding this comment.
Hmm, this prevents importing PKCS#12 files with empty passwords (which are valid, I think). The dyncerts.py:202 and nss_ssl.py:1219-1220 handle this correctly by defaulting to empty string though.
There was a problem hiding this comment.
Yes are correct, this check is too strict. I will use the dyncerts.py method of import a p12 file, with or without password.
src/lib389/lib389/nss_ssl.py
Outdated
| self._assert_not_chain(cert_file) | ||
|
|
||
| is_ca_cert = cert_is_ca(cert_file) | ||
| if ca and not is_ca_cert: |
There was a problem hiding this comment.
It's a duplicate -- previously in the same function there's already the check -- nss_ssl.py:1195-1210
src/lib389/lib389/dyncerts.py
Outdated
| if cert._dn == self._basedn: | ||
| continue | ||
| der_cert = cert.get_attr_vals_bytes(DYCATTR_CERTDER)[0] | ||
| log.info(f"JC der_cert len:{len(der_cert)}") |
src/lib389/lib389/nss_ssl.py
Outdated
|
|
||
| def add_cert(self, nickname, input_file, ca=False): | ||
| def add_cert(self, nickname: str, cert_file: str, ca=False, | ||
| pkcs12_password=None, force: bool = False): |
There was a problem hiding this comment.
Both - this function and add_ca_cert() - they use force but actually it's not used. It's for a future?
There was a problem hiding this comment.
In the case where the dyncerts instance is offline we use NssSsl, but IIUC there is no force option for importing a cert into the NSS DB.
jchapma
force-pushed
the
hot-cert-phase4
branch
from
bcdd7c6 to
780c905
Compare
display, ca cert bundles
jchapma
force-pushed
the
hot-cert-phase4
branch
from
780c905 to
4aad584
Compare
|
Regarding the sourcery blocking security issues: These are a false positive for command injection, subprocess.check_output() is called with a list of args, there is no shell parsing as shell=false by default. The list of args are passed to certutil, so user controlled values cannot trigger command injection. |
progier389
left a comment
progier389
left a comment
There was a problem hiding this comment.
Looks good but there are still some issues:
- dsconf supplier1 security ca-certificate list
- dsconf supplier1 security certificate list
lists twice the certificates:
dsconf supplier1 security ca-certificate list
Certificate Name: Self-Signed-CA
Subject DN: CN=ssca.389ds.example.com,O=testing,L=389ds,ST=Queensland,C=AU
Issuer DN: CN=ssca.389ds.example.com,O=testing,L=389ds,ST=Queensland,C=AU
Expires: 2028-01-29 14:03:05
Trust Flags: CT,,
Certificate Name: Self-Signed-CA
Subject DN: CN=ssca.389ds.example.com,O=testing,L=389ds,ST=Queensland,C=AU
Issuer DN: CN=ssca.389ds.example.com,O=testing,L=389ds,ST=Queensland,C=AU
Expires: 2028-01-29 14:03:05
Trust Flags: CT,,
Certificate Name: NewCA
Subject DN: C=FR,O=test-ECDSA-CA,CN=ibm-p8-kvm-03-guest-02.virt.pnr.lab.eng.rdu2.redhat.com
Issuer DN: C=FR,O=test-ECDSA-CA,CN=ibm-p8-kvm-03-guest-02.virt.pnr.lab.eng.rdu2.redhat.com
Expires: 2036-01-27 14:49:25
Trust Flags: CT,,
Certificate Name: NewCA
Subject DN: C=FR,O=test-ECDSA-CA,CN=ibm-p8-kvm-03-guest-02.virt.pnr.lab.eng.rdu2.redhat.com
Issuer DN: C=FR,O=test-ECDSA-CA,CN=ibm-p8-kvm-03-guest-02.virt.pnr.lab.eng.rdu2.redhat.com
Expires: 2036-01-27 14:49:25
Trust Flags: CT,,
A leftover debug trace displayed the certs twice, removed it. |
droideck
left a comment
droideck
left a comment
There was a problem hiding this comment.
Additional test coverage would be helpful - a few areas caught my eye that might benefit from closer review, though it's hard to say for certain without tests to explore them further.
Since this touches a sensitive security area, I think we should consider expanding the test suite before releasing this feature. Of course, as a separate PR.
Nice to cover areas, IMO:
- Invalid/malformed certificate files (corrupted PEM, truncated DER, invalid PKCS#12)
- PKCS#12 with wrong password, missing password, empty password
- Overwriting existing certificates (with and without --force)
- Backend switching and consistency behavior (DynamicCerts vs NssSsl)
- JSON vs text output consistency across all list/get commands
- DynamicCerts timestamp normalization
src/lib389/lib389/nss_ssl.py
Outdated
| "-W", pkcs12_password, | ||
| ] | ||
|
|
||
| self.log.info(f"nss import p12 cmd: {format_cmd_list(cmd)}", ) |
There was a problem hiding this comment.
I think we should mask the password here (now it's printed plain text in the logs).
| output = _dump_cert(cert, json_output=args.json) | ||
| print(json.dumps(output, indent=4)) | ||
| _dump_cert(cert, json_output=args.json) |
There was a problem hiding this comment.
When it's JSON, it'll call it twice (same in cacert_get)
Also I think it'll be silent in non-JSON mode (no log.info). Is it intentional?
| cacert_add_parser.add_argument('--file', required=True, | ||
| help='Sets the file name of the CA certificate') | ||
| cacert_add_parser.add_argument('--name', nargs='+', required=True, | ||
| help='Sets the name/nickname of the CA certificate, if adding a PEM bundle then specify multiple names one for ' |
There was a problem hiding this comment.
I think it's a wrong docstring typo as it says Sets the name/nickname while it's under --file.
| log.info('Expires: {}'.format(details[3])) | ||
| log.info('Trust Flags: {}'.format(details[4])) | ||
| output = _dump_cert(cert, json_output=args.json) | ||
| print(json.dumps(output, indent=4)) |
There was a problem hiding this comment.
Why it was changed to print? I think we should use log.info.
src/lib389/lib389/dyncerts.py
Outdated
| if not der_cert: | ||
| raise ValueError(f"Failed to extract DER bytes from {cert_file}") | ||
|
|
||
| dn = f"cn={nickname},{self._basedn}" |
There was a problem hiding this comment.
I think we should use ldap.dn.escape_dn_chars() here for safety.
| """ | ||
| try: | ||
| year = int(raw[0:4]) | ||
| month = int(raw[4:6]) + 1 # PRExplodedTime.tm_month is 0–11 |
There was a problem hiding this comment.
One of the places where I think we'll need to have test coverage. I worry that there can be cases where the dates are shifted by 1 month.
There was a problem hiding this comment.
dyncerts timestamps use 0 based months:
dsdynamiccertificatenotbefore: 20260006181934Z
dsdynamiccertificatenotafter: 20270006181934Z
This is why we month +1 in _normalise_timestamp, without it, January would be month 0, which is invalid for datetime.
src/lib389/lib389/utils.py
Outdated
| with open(cert_file, "rb") as f: | ||
| data = f.read() | ||
| except ValueError as ve: | ||
| raise ValueError(f"Unable to load certificate '{cert_data}': {ve}") |
There was a problem hiding this comment.
IIRC, open() raises FileNotFoundError/IOError, not ValueError (also, OSError?)
| certmgr.del_cert(args.name) | ||
| log.info(f"Successfully deleted certificate") | ||
| except ValueError as e: | ||
| log.info(f"Failed to delete certificate '{args.name}': {e}") |
There was a problem hiding this comment.
I think this should be a log.error().
I agree, I think Pierre is working on CI tests. |
Indeed. I will update the Jira ticket with these interesting comments |
droideck
left a comment
droideck
left a comment
There was a problem hiding this comment.
Besides a few small issues - LGTM!
| else: | ||
| print(msg) |
There was a problem hiding this comment.
I am not sure I understand the purpose here...
|
|
||
| if args.json: | ||
| output = [_dump_cert(cert, json_output=True) for cert in certs] | ||
| print(json.dumps(output, indent=4)) |
There was a problem hiding this comment.
I think it should be log.info() as it was before.
|
|
||
| if args.json: | ||
| output = [_dump_cert(cert, json_output=True) for cert in ca_certs] | ||
| print(json.dumps(output, indent=4)) |
| help='Sets the name/nickname of the certificate') | ||
| cert_add_parser.add_argument('--primary-cert', action='store_true', | ||
| help="Sets this certificate as the server's certificate") | ||
| cert_add_parser.add_argument('--pkcs12-pin-text', help='The PKCS#12 password as plain text') |
There was a problem hiding this comment.
I think it's worth to add an additional warning here about the fact that the password will be displayed in ps aux and shell history. So the warning should advise to use stdin and file options which are more secure.
| der_privkey = privkey.private_bytes( | ||
| encoding=serialization.Encoding.DER, | ||
| format=serialization.PrivateFormat.PKCS8, | ||
| encryption_algorithm=serialization.NoEncryption() |
There was a problem hiding this comment.
We might need to document that the private keys are stored unencrypted in LDAP. So anyone with read access to cn=dynamiccertificates can extract private keys (so it should be protected by ACI always). Or/and we can add encryption. Otherwise, I'll leave it up to you to decide the approach here. :)
| if cert_file and cert_file.lower().endswith((".p12", ".pfx")): | ||
| try: | ||
| privkey, cert, _ = pkcs12.load_key_and_certificates( | ||
| data, pkcs12_password.encode() if pkcs12_password else None, |
There was a problem hiding this comment.
The function's type hint says bytes but encode() will fail if called on bytes. Consider using ensure_bytes() here.
4 participants
Desciption:
Add the CertManager abstraction layer and DynamicCerts backend module. Enhance the NssSsl backend to support importing PKCS#12 containers, containing cert and private key. Update dsconf to support new PKCS#12-related args, allowing users to supply passwords via text, stdin, or file.
Fix:
Relates: #6951
Reviewed by:
Summary by Sourcery
Introduce a certificate management abstraction that can use either the DynamicCerts LDAP backend or the NSS database, and extend CLI and SSL tooling to support PKCS#12 certificate imports.
New Features:
Bug Fixes:
Enhancements:
Tests: