Skip to content
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 IPAddrBlock and ASIdentifiers extensions (RFC 3779) #4699

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arckoor
Copy link
Contributor

@arckoor arckoor commented Feb 19, 2025

Adds support for RFC 3779.
Continuation of #4443

@coveralls
Copy link

coveralls commented Feb 19, 2025

Coverage Status

coverage: 91.623% (+0.1%) from 91.526%
when pulling 62cf9db on arckoor:rfc-3779
into 399720d on randombit:master.

@randombit randombit self-requested a review February 19, 2025 22:31
@randombit randombit added this to the Botan 3.8.0 milestone Feb 19, 2025
Copy link
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some parts of this I don't understand, I still need to read the RFC, so this isn't quite a complete review.

@arckoor arckoor force-pushed the rfc-3779 branch 2 times, most recently from c43c646 to 2cc3382 Compare March 4, 2025 13:10
@arckoor arckoor requested a review from randombit March 4, 2025 13:54
Copy link
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the comments. This is looking quite close. For my part I still need to review the RFC in order to understand the logic with merging/sorting.

@randombit
Copy link
Owner

More of a meta question - RFC 3779 defines (well, at least alludes to) how to verify this extension, in section 3.3. Is implementing this verification as part of the current path verification logic planned, perhaps in a later PR? Or were you planning on verifying yourselves out of band, after standard path verification? Either is fine; the first is preferable but if the second it should be documented that these extensions are exposed for informational purposes but are not examined as part of path verification.

@arckoor
Copy link
Contributor Author

arckoor commented Mar 7, 2025

Verification is planned as part of path verification. We can try and add it to this PR already, or add it to the FFI PR that should also come eventually (or do it entirely separate).

@@ -67,6 +67,8 @@ enum class Certificate_Status_Code {
CERT_CHAIN_TOO_LONG = 4002,
CA_CERT_NOT_FOR_CERT_ISSUER = 4003,
NAME_CONSTRAINT_ERROR = 4004,
IPADDR_BLOCK_ERROR = 4011,
ASBLOCKS_ERROR = 4012,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We weren't really sure what to call the errors, nor what kind of error messages to use. We think it's definitely a validation error, but unfortunately there wasn't any space left within the enum, so now the numbers are kind of awkward.

@arckoor
Copy link
Contributor Author

arckoor commented Mar 13, 2025

@randombit We've added path validation for both extensions. The IP Address one is still missing a more thorough failure test, which we will add... eventually, hopefully by the end of the month. We've pushed our current changes for now, so you can hopefully already take a look.

fmt("IP address range entries must have a length between 1 and {} bytes.", static_cast<uint8_t>(V)));
}

addr_min.erase(addr_min.begin()); // remove 'unused' octet
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further down for the max address we check that if max_addr.empty() then unused must be 0. We need to check this because we need to set the unused bits to 1. For the min address however they are already 0, so until now we've just ignored the unused octet, as we don't need to examine it. Should we still check it? In the case that unused != 1 and addr_min.empty() it would technically be a "bad" encoding - I'm just not sure if the user should be made aware of that.

@arckoor
Copy link
Contributor Author

arckoor commented Mar 26, 2025

@randombit Path validation is now tested more thoroughly. We've also left comments on some parts of the code where we're unsure, feedback there would also be appreciated c:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants