Snow 2401045 fix vulnerabilities in crl #2584
Conversation
sfc-gh-pczajka
added
NO-CHANGELOG-UPDATES
Co-authored-by: Tomasz Urbaszek <[email protected]>
Co-authored-by: Tomasz Urbaszek <[email protected]>
src/snowflake/connector/crl.py
Outdated
| # Check if start certificate is expired | ||
| if not self._is_valid(start_cert): |
There was a problem hiding this comment.
nit/optional: I would be tempted to move away from is_valid which generally means more than simply having correct dates. The documentation is very helpful regardless.
Maybe _is_within_lifetime or _is_within_validity_dates?
There was a problem hiding this comment.
Nice, changed
| if not self._is_ca_certificate(cert): | ||
| logger.warning("Ignoring non-CA certificate: %s", cert) | ||
| continue |
There was a problem hiding this comment.
We should also be checking the lifetimes of the intermediates.
I realize we are almost certainly not going to get attacked by an expired leaked intermediate, but it should be cheap to add and would be closer to spec.
There was a problem hiding this comment.
Thanks for the catch! I've overlooked this - fixed the check and tests
src/snowflake/connector/crl.py
Outdated
| if issue_date.tzinfo is None: | ||
| issue_date = issue_date.replace(tzinfo=timezone.utc) | ||
| issue_date, expiry_date = CRLValidator._get_certificate_validity_dates(cert) | ||
| validity_period = expiry_date - issue_date |
There was a problem hiding this comment.
nit: the notAfter date is inclusive so this calculation is off by one [text from RFC 5280]. I think your code falls on the safe side and CAs worth their salt say away from the edges, but we should probably fix it to show competence.
3 participants
Fix CRL implementation vulnerabilities described in SNOW-2401045
basicConstraints isCA = truefor all certificates except a leaf one