-
-
Notifications
You must be signed in to change notification settings - Fork 612
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
Add CAA presence/validity information to ValidationRecords #2165
Comments
This should probably also contain the the highest label at which a record was found if one is found at all. |
I would be interested in trying to do this. Do you have any directions/hints? |
@rolandshoemaker I would be interested in implementing this. Is it still reverent? |
@rolandshoemaker @jsha I would be interested in implementing this, so I was wondering if it is still relevant, and what information you think would be good to keep in the validation records. |
Hey! Sorry for the silence, I completely missed your comments. This is still relevant and it would be great if you had a chance to take a stab at it. I think it makes sense to just include the CAA information for now. |
No problem. So from my understanding of the issue you would want validation record similar to below for now? Do you have any preferences on the format? [
{
"url":"http://c185da94d854a28961368b75dd634b29afcf7f60.com:5002/.well-known/...",
"hostname":"c185da94d854a28961368b75dd634b29afcf7f60.com",
"port":"5002",
"addressesResolved":["127.0.0.1"],
"addressUsed":"127.0.0.1"
},
{
"caaStatus": "none|valid|invalid"
}
] |
Records CAA status (i.e. missing, found and valid, or invalid) in Validation Records. Functionally is gated behind a feature flag, and defaulted to off. Tests included for both states of the flag. Issue letsencrypt#2165
👋 @laurencemcf - thanks for your interest in taking this issue! I talked a bit with @rolandshoemaker out of band. I think we've both come around to the consensus the CAA information should be captured at the authorization level and not in the authorization's validation records. This slightly complicates the work involved as it would require a database migration to add a new column to the The other "catch" related to this work is when an issuance for an identifier is backed by a valid authorization older than 8hrs we need to recheck CAA per the BRs and should also update the authorization's CAA state at that time. That makes CAA information a many-to-one relation with an authorization. I apologize that I'm providing this guidance after you've already taken a crack at an implementation in #3806. Does the above make sense to you? Are you interested in closing #3806 and resubmitting a PR based on the above? Thanks! |
@cpu No problem, I'll close #3806 and have a go at doing this way. So to confirm we want to add a new json column to the A example of the format of the new column:
|
@laurencemcf Great, thanks!
I think we should avoid JSON columns. A separate table for CAA information would be better normalized and easier to query against. @rolandshoemaker do you concur? @rolandshoemaker Adding an empty field to the pendingAuthz table is awkward... Do you have any thoughts there? In theory that field will get removed when we finally consolidate the two tables so maybe its a fair compromise. |
If we want to have a separate table for CAA, then due to the many-to-one relationship I think we can avoid having a column in either CREATE TABLE `caa` (
`id` varchar(255) NOT NULL,
`authzID` varchar(255) DEFAULT NULL,
`status` varchar(255) DEFAULT NULL,
`identifier` varchar(255) DEFAULT NULL,
`timestamp` datetime DEFAULT NULL,
PRIMARY KEY (`id`),
CONSTRAINT `caa_authz` FOREIGN KEY (`authzID`) REFERENCES `authz` (`id`)
) |
@laurencemcf Of course! That makes sense :-) Thanks for pointing that out. @rolandshoemaker Are you OK with the separate table approach? |
I think this approach makes sense, in the past we've been way too willing to just keep stuffing stuff in existing tables when a new table would make more sense. In terms of the schema I think the ID should be a Probably we'll also want a |
@rolandshoemaker I think it might be prudent to put an index on the |
Hm, good point. We may want to change the name of the |
@rolandshoemaker makes sense, but I was just wondering what we should do for the Also is there any other information which would be useful to include in this table, e.g. whether it was a wild card check, or the validation methods allowed? I don't think either of those things would be much work to add but may make the table more useful depending on the querying patterns you envision. |
I've been working on implementing this and had a question about the best way to do it. We need to add a new RPC between the RA and SA to save the CAA recheck information (for authorizations older than 8hrs), and I was wondering if we should use this new RPC when we perform the initial validation as well. At the time of the initial validation we are already calling the |
Yup null makes sense in that case.
Probably makes sense to include if it was a wildcard check, other than that I can't think of anything in particular, given our parameter work is still being changed I think it should be left out of any initial implementation and we can extend the table at a later date if we want to include that info.
I'd need to take a closer look but I believe we can just extend the existing CAA recheck RPC response to include the information we want back. We can also extend the FinalizeAuthorization RPC request to include this information and simply alter the SA method to also store that (I'd also not be strictly opposed to simply doing two RPCs here from RA -> SA, which would be a slightly cleaner boundary, but since we will only ever be doing this after a FinalizeAuthorization call I think it's fine to just mash them together). |
Since this is important information about an authorization.
The text was updated successfully, but these errors were encountered: