PKI - Honor header If-Modified-Since if present #16249
Conversation
|
Hello @cipherboy, opened this pull request just to make sure that what we have discussed for #3190 is still valid since there seem to have been many changes in the crl endpoints. I still intended to test the implementation and also look at a possible refactoring of the patchFetch function. |
|
Hi @Gabrielopesantos - please don't forget to add a changelog entry too. :) Please see the contributing.md for more details. |
cipherboy
changed the title
|
Sorry @Gabrielopesantos -- I've been on vacation for a bit, I'll take a look at this week now that I'm back :) |
There was a problem hiding this comment.
This looks great! I think you'd want to do it for pathGetIssuerCRL and perhaps pathGetRawIssuer as well? :-) You might need to store the creation time on the IssuerEntry object as well, perhaps migrating if one doesn't exist. I wouldn't do the CA if-modified-since on the default issuer (path_fetch.go).
There was a problem hiding this comment.
Many thanks! I think this looks great! :-)
A few comments inline, perhaps @stevendpclark could take a look too. But I think this is ready to unmark from draft and we'll merge it.
I've think we've got some changes in the pipeline that'll probably cause merge conflicts -- would you like us to resolve those or would you like to? If we merge this sooner rather than later, we can definitely make this release and perhaps avoid some of those conflicts... :-D
Happy to write the tests if you don't want to, too. :-)
|
Ok, with this last commit I am happy with opening the pull request. I have made a small refactoring based on comments which I think is an improvement from what I had before. I think some of the stuff related to checking the header can moved from storage.go to a utils file (not sure what to call it). There is still also maybe an update in the Docs. Regarding tests, I wouldn't minding doing them as there are still likely corner cases that I did not manually test or don't know about. However, as we might have merge conflicts and if you think we can make this release, I'm ok with other person taking from here. Let me know if there is anything else I can do regarding this PR, if not I will just come in the end to set what were your changes. |
cipherboy
force-pushed
the
pki/honor-header-http-if-modified-since
branch
from
247697b to
a9cf675
Compare
For the most part, these take a SC as param, but aren't directly storage relevant operations. Move them out of storage.go as a result. Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
When the default is updated, access under earlier timestamps will not work as we're unclear if the timestamp is for this issuer or a previous issuer. Thus, we need to invalidate the CRL and both issuers involved (previous, next) by updating their LastModifiedTimes. Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
When the default issuer changes, we'll have to mark the invalidation on PR secondary clusters, so they know to update their CRL mapping as well. The swapped issuers will have an updated modification time (which will eventually replicate down and thus be correct), but the CRL modification time is cluster-local information and thus won't be replicated. Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
cipherboy
force-pushed
the
pki/honor-header-http-if-modified-since
branch
from
a9cf675 to
aabf2c7
Compare
Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
|
A few follow-up minor changes can be merged in the next PR. Thank you @Gabrielopesantos for kicking this off and doing a lot of the work in this PR!! |
No description provided.