-
Notifications
You must be signed in to change notification settings - Fork 85
Update RFC5280Policy and OCSPVerifierPolicy initializers #276
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
Update RFC5280Policy and OCSPVerifierPolicy initializers #276
Conversation
New initializers for `ExpiryPolicy`, `RFC5280Policy`, and `OCSPVerifierPolicy` were introduced in apple#266. These initializers were added to prevent users from specifying `Date.now` as the time to validate certificate expiry against. For context, the problem was that `Date.now` would be evaluated at initialization, but validation could occur at a later stage, at which point the validation time would become stale. These initializers had a `fixedValidationTime: Date? = nil` argument, where a non-`nil` value denoted a *fixed* time to validate against, and a `nil` value denoted that the current time, evaluated at the point of validation, would be used for validation (the common case). Although the dangers of specifying `fixedValidationTime = .now` were documented, in practice, it is very easy for users to miss this and continue using the API in an incorrect way. Given that most users will validate certificate expiry against the current time, we want to default to this and remove the validation time argument from the public initializers entirely. Users who want to specify a fixed predetermined time must explicitly opt-in by using `@_spi(FixedValidationTime)` to access the initializers that have a *non-optional* `fixedValidationTime` argument. Modifications: - Deprecated the initializers of `RFC5280Policy` and `OCSPVerifierPolicy` and introduced two new initializers for each: one without a validation time argument (common case), and one with a *non-optional* validation time argument backed behind an SPI - Updated the initializers of `ExpiryPolicy` (an internal type) - Additional changes: - Updated test cases to use new initializers - Updated examples in documentation to use new initializers Result: `RFC5280Policy` and `OCSPVerifierPolicy` can be used more safely
Sources/X509/OCSP/OCSPPolicy.swift
Outdated
/// - Warning: Only use this initializer if you want to validate the certificates against a *fixed* time. Most users | ||
/// should use ``init()``: the expiry of the certificates will be validated against the current time (evaluated at | ||
/// the point of validation) when using that initializer. | ||
@_spi(FixedValidationTime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not convinced that we should be using SPI
for this. SPI is an undocumented feature and hard to discover. What if we either:
- Make this a factory method instead of an init so it is harder to discover and gets a concrete name
- Use package traits to guard this API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making this hard to discover is absolutely intentional. We changed the behaviour, but users continued to misuse it, so we're trying to take this API away from almost everyone.
/// - Parameter requester: A requester instance conforming to ``OCSPRequester``. | ||
/// - Parameter fixedValidationTime: The *fixed* time to compare against when determining if the request is recent. A fixed time is a *specific* | ||
/// time, either in the past or future, but **not** the current time. To compare against the current time *at the point of validation*, pass `nil` to | ||
/// `fixedValidationTime`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please restore these doc comments to the deprecated methods?
Motivation:
New initializers for
ExpiryPolicy
,RFC5280Policy
, andOCSPVerifierPolicy
were introduced in #266. These initializers were added to prevent users from specifyingDate.now
as the time to validate certificate expiry against. For context, the problem was thatDate.now
would be evaluated at initialization, but validation could occur at a later stage, at which point the validation time would become stale.These initializers had a
fixedValidationTime: Date? = nil
argument, where a non-nil
value denoted a fixed time to validate against, and anil
value denoted that the current time, evaluated at the point of validation, would be used for validation (the common case). Although the dangers of specifyingfixedValidationTime = .now
were documented, in practice, it is very easy for users to miss this and continue using the API in an incorrect way.Given that most users will validate certificate expiry against the current time, we want to default to this and remove the validation time argument from the public initializers entirely. Users who want to specify a fixed predetermined time must explicitly opt-in by using
@_spi(FixedExpiryValidationTime)
to access the initializers that have a non-optionalfixedExpiryValidationTime
argument.Modifications:
RFC5280Policy
andOCSPVerifierPolicy
and introduced two new initializers for each: one without a validation time argument (common case), and one with a non-optional validation time argument guarded behind an SPIExpiryPolicy
(an internal type)Result:
RFC5280Policy
andOCSPVerifierPolicy
can be used more safely