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 SBGP extension handling #2053

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Conversation

thillux
Copy link

@thillux thillux commented Oct 12, 2023

This PR adds support for parsing SBGP ASNs and IP-Ranges. A basic test is also added.

@wiktor-k
Copy link
Contributor

FTR I think the job fails because of issues fixed in #2052 (just saying that it's nothing that you've introduced).

@thillux
Copy link
Author

thillux commented Oct 12, 2023

FTR I think the job fails because of issues fixed in #2052 (just saying that it's nothing that you've introduced).

Thank you for the hint!

@thillux thillux force-pushed the sbgp-extension branch 2 times, most recently from 3bf5d75 to d759114 Compare October 12, 2023 14:27
@thillux
Copy link
Author

thillux commented Oct 12, 2023

This currently depends on #2055. After it is merged, I'll cleanup my temporary commits.

Signed-off-by: Markus Theil <theil.markus@gmail.com>
@thillux thillux changed the title Add S-BGP extension handling Add SBGP extension handling Oct 12, 2023
Signed-off-by: Markus Theil <theil.markus@gmail.com>
@thillux
Copy link
Author

thillux commented Oct 14, 2023

This currently depends on #2055. After it is merged, I'll cleanup my temporary commits.

I found another fix for the compilation with Rust 1.56. This PR is ready now from my side.

* At the moment this is coded for simplicity, not speed.
*/
#[cfg(ossl110)]
fn addr_expand(addr: *mut u8, bs: *const ASN1_BIT_STRING, length: isize, fill: u8) -> bool {
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't seem like something that should be in openssl-sys.

Choose a reason for hiding this comment

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

This function, and associated functions , has now been replaced with a FFI binding to X509v3_addr_get_range and X509v3_addr_get_afi.

};
append(&mut value, &mut first, true, &asn_format);
}
X509Extension::new_nid(None, Some(ctx), Nid::SBGP_AUTONOMOUSSYSNUM, &value)
Copy link
Owner

Choose a reason for hiding this comment

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

We need to move away from the string APIs for x509 extensions, and instead directly work with the relevant OpenSSL types.

}
}

pub fn ranges(&self) -> Option<Vec<(u32, u32)>> {
Copy link
Owner

Choose a reason for hiding this comment

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

Does this correspond to some OpenSSL function?

Copy link
Owner

Choose a reason for hiding this comment

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

Same question for the other methods in this file.

Choose a reason for hiding this comment

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

These function correspond to X509v3_addr_get_range, but executed for the whole extension, since RFC 3779 encodings are rather tedious to work with. Same thing applies to the other functions in this file, they just parse the openssl-internal representation into their Rust-equivalents.

@botovq
Copy link
Contributor

botovq commented Oct 21, 2023

This is a very dangerous API full of traps and bugs. It needs to be wrapped very carefully. What is the use case you want to support by exposing these? Do you have some working code I could look at?

@thillux
Copy link
Author

thillux commented Nov 15, 2023

@botovq We are currently in the process of reworking this PR. Our use case is certifying address ranges and ASNs in a datacenter fabric setting. Sadly I have no public code that I can share.

@PetrichorIT is currently working on this.

Copy link
Contributor

@botovq botovq left a comment

Choose a reason for hiding this comment

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

I really think it is a bad idea to expose this API. What stops you from using rpki-rs for this?

As already mentioned, and you will have no doubt found while developing this, the OpenSSL RFC 3779 API is really bad. And I am not aware of any OpenSSL developer with domain expertise to speak of. So that's not going to change anytime soon.

You don't magically get memory safety by using this via ffi from Rust and then omitting error checks for functions that allocate internally. Very much the opposite of that.

There are omissions of necessary features, missing checks, etc. This all looks taylored to a narrow use case. I do not think this is anywhere close to being ready. People who will try to use this in the future will have a very hard time interoperating with modern RPKI validators.

/// Construct a new `SbgpAsIdentifier` extension.
pub fn new() -> SbgpAsIdentifier {
SbgpAsIdentifier {
critical: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should ever be non-critical. At least default to critical since that's what RPKI certs should contain. It's a MUST in RFC 6487 section 2, while RFC 3779 marks it as SHOULD, hence false is a poor default.

Choose a reason for hiding this comment

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

yes, critical=true is now the default

}

/// Sets the `critical` flag to `true`. The extension will be critical.
pub fn critical(&mut self) -> &mut SbgpAsIdentifier {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this knob at all?

Choose a reason for hiding this comment

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

In its newer version fn critical(&mut self, value: bool) is might be useful to have this configuration option for some legacy systems ? I personally cannot think of a use case, but just in case?

if min == max {
ffi::X509v3_asid_add_id_or_range(asid, 0, min.as_ptr(), std::ptr::null_mut());
} else {
ffi::X509v3_asid_add_id_or_range(asid, 0, min.as_ptr(), max.as_ptr());
Copy link
Contributor

Choose a reason for hiding this comment

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

These can fail and will sometimes free min and/or max and sometimes they won't. In the former case you'll double free below.

Definitely needs error checking, but I'm unsure how you can avoid the double free without panicking and retain memory safety.

Choose a reason for hiding this comment

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

The newer implementation should prevent double frees + free all nessecary allocations.

/// Construct a new `SbgpIpAddressIdentifier` extension.
pub fn new() -> SbgpIpAddressIdentifier {
SbgpIpAddressIdentifier {
critical: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, default to critical

std::mem::forget(max);
}

X509Extension::new_internal(Nid::SBGP_AUTONOMOUSSYSNUM, self.critical, asid as *mut _)
Copy link
Contributor

Choose a reason for hiding this comment

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

Before doing this you should check that your extension data is canonical, otherwise you produce an extension with invalid DER encoding.

self.asn.push((asn_min, asn_max));
self
}

Copy link
Contributor

Choose a reason for hiding this comment

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

add_inherit() is missing in the builder.

use libc::*;

#[repr(C)]
#[cfg(ossl110)]
Copy link
Contributor

Choose a reason for hiding this comment

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

All these should probably condition on OPENSSL_NO_RFC3779 as well.

@PetrichorIT
Copy link

Given the current state of the RFC 3779 API, I think there are two options:

Either we only expose a certain subset of the RFC 3779 API that we can assure to be "safe", or we expose the entire API with all meaningful features and checks and try to build a more abstract API around it, to ensure safety.

I personally prefer a second approach.
If we limit the ways we use the API surface of openssl when interacting with the prime suspects
(X509v3_asid_add_id_or_range, ...), I think the API could be safely represented in Rust.
For the time being, separating the creational API and the read API seems like a good idea
to manage the complexity inherent to this problem.

I'd like to try this approach and see where it leads.

@thillux
Copy link
Author

thillux commented Nov 29, 2023

I really think it is a bad idea to expose this API. What stops you from using rpki-rs for this?

  • We have custom OpenSSL providers for random number generation we'd like to use here.
  • We already use OpenSSL with RFC3779 support in a C++-based application, which consumes certificates from our Rust-based CA.

As already mentioned, and you will have no doubt found while developing this, the OpenSSL RFC 3779 API is really bad. And I am not aware of any OpenSSL developer with domain expertise to speak of. So that's not going to change anytime soon.

  • We are aware of this and never use OpenSSL without address sanitizers as the bare minimum during development in our C++ applications using OpenSSL and RFC3779.

You don't magically get memory safety by using this via ffi from Rust and then omitting error checks for functions that allocate internally. Very much the opposite of that.

  • Sure, due to memory safety reasons we chose Rust for our CA application, but it does not completely outweigh missing features in the OpenSSL API subset exposed for our use case.

There are omissions of necessary features, missing checks, etc. This all looks taylored to a narrow use case. I do not think this is anywhere close to being ready. People who will try to use this in the future will have a very hard time interoperating with modern RPKI validators.

  • We currently do not deploy a full RPKI and are a bit taylored yes. But we are eager to make this PR usable in a broader fashion. If somebody steps in and tells us, what is missing in his opinion.

Copy link
Contributor

@botovq botovq 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 the explanations. I won't have time for a thorough review in the next days/week, so I only left a few quick comments.

I think we should aim at a good middle ground between the two options you describe: let's make it flexible enough to be generally usable and correct, but let's make it easy enough to use by not exposing things that aren't used in the wild.

So, for example, you currently don't expose the SAFI for IPAddressFamily and you don't expose the RDI variant for ASIdentifiers (which I think is good). In a similar vein, let's not expose criticality as a knob.

pub struct SbgpAsIdentifier {
critical: bool,
asn: Vec<(u32, u32)>,
inherit: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is good hygiene to make illegal states non-representable as far as possible/reasonable. The ASIdentifiers should either inherit or contain a (non-empty) list of ASIds and ASRanges. So I think this should be an enum with an Inherit and an ASNumber variant that contains the Vec<(u32, u32)> with the list of ASIds and ASNumbers.

Choose a reason for hiding this comment

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

I think this is a good idea. However I think patterns like type-state would be overkill in this scenario, so assertions / panics remain nessecary to prevent silent failures.

@@ -428,6 +443,258 @@ impl AuthorityKeyIdentifier {
}
}

#[cfg(ossl110)]
pub struct SbgpAsIdentifier {
critical: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest that you omit this field. If anyone really needs to fiddle with criticality, they can readily add the field and accessor with a solid justification.

pub struct SbgpIpAddressIdentifier {
critical: bool,
ip_ranges: Vec<(IpAddr, IpAddr)>,
inherit: Option<IPVersion>,
Copy link
Contributor

@botovq botovq Nov 29, 2023

Choose a reason for hiding this comment

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

Similarly here, let's omit the criticality marker and for the sake of correctness, I think this should have two optional enums, one for IPv4 and one for IPv6. Each of these enums contains either an Inherit marker or a vector of IP ranges of the correct kind.

Unless I'm missing something, this struct doesn't currently allow representing inheritance of both IPv4 and IPv6.

@PetrichorIT
Copy link

So this PR should be ready now.

In regards to the extension builders, criticality will no longer be a configuration option,
and all builder should now make invalid states irrepresentable. To prevent missuse of the current
API , builders will panic if an operation results in a loss of information, so e.g.

SbgpAsIdentifier::new()
    .add_asn(1234)
    .add_inherit()
    .build()

would panic, since the assigned AS number would be lost when the inherit flag is set.

The functions X509V3_{addr, asid}_{inherits, subset, is_canonical} are now represented in the API.
X509V3_{addr, asid}_canonize is not publicly exposed, since the builder only needs to use it internally.
In general, i think is good to keep the read API and write/builder API seperate, since mutating a raw extension
could go wrong in a number of ways and we dont really want to expose the underlying datastructures to the user.

In regards to X509V3_asid_subset, since this API is faulty in 1.1.0, it was moved to 1.1.1 (openssl/openssl#18514).

The APIs for interacting with store contextes and validation paths X509v3_{asid, addr}_{validate_path, validate_resource_set}
are not supported, since they seem like a source of unwanted complexity regarding this PR. They are however bound in openssl-sys,
for completions sake.

@botovq

@botovq
Copy link
Contributor

botovq commented Feb 1, 2024 via email

@botovq
Copy link
Contributor

botovq commented Feb 29, 2024

Apologies for the delay. I haven't forgotten. I won't be able to review this properly before mid-March.

@HolyShitMan
Copy link

Apologies for the delay. I haven't forgotten. I won't be able to review this properly before mid-March.

Did you find any time to review the PR?

Greetings

@botovq
Copy link
Contributor

botovq commented Mar 26, 2024 via email

@HolyShitMan
Copy link

Knowing that my request may be annoying: I would like to kindly remind you that this pull request is still open.

Copy link
Contributor

@botovq botovq left a comment

Choose a reason for hiding this comment

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

Ignoring the comment on X509_get_ext_d2i(), the use of the C API looks about as sane as it can be now.

Regress coverage is decent. As noted, various panic strings need a typo fix (allready -> already). Whether rust-openssl would like panics or different error handling is up to the maintainers of the crate -- as are other API concerns on the rust side of things.

As far as the reservations I had are concerned, this should be ready to proceed.

ffi::NID_sbgp_autonomousSysNum,
std::ptr::null_mut(),
std::ptr::null_mut(),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

X509_get_ext_d2i is a tricky API to use correctly. As far as I can see, no caller in rust-openssl gets it right.

This can return NULL for several reasons: the benign reason is that there is no extension corresponding to the nid in the cert. However, there could also be a library-internal error (malformed extension, memory allocation error, ...), or other errors, like multiple extensions of the same type, which this idiom silently ignores.

The correct way is to pass in a pointer crit to a c_int as third argument. If NULL is returned and crit is set to anything but -1, an error should be raised.

Not sure if this should be fixed in this PR or if rust-openssl wants to land this as-is and fix the issue in a sweep.

Choose a reason for hiding this comment

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

After considering this further, if there would be a minimally invasive solution available, I'd opt to address it within this PR. However, it seems that we require some new Error construct due to the fact that X509_get_ext_d2i doesn't set the internal OpenSSL error, making the ErrorStack inapplicable.

I'm unsure if this issue extends to other OpenSSL functions as well, where a new Error construct would also help to handle return values properly. After all, I propose addressing this in a separate PR to avoid overloading the current one.

Choose a reason for hiding this comment

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

In the meantime of the last few months we also got some additional X509 functionality.
Yet, I suggest following the same procedure: let's complete the current task first, and then I'll create an additional PR specifically for the new functions.

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@HolyShitMan I'm inclined to agree, but it isn't my call. I opened #2226, which, once addressed, should also fix the instance in this PR.

Choose a reason for hiding this comment

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

@botovq: #2226 and it's solution proposed in #2240 have been open for over a month without any significant progress. Could we find a way to expedite this? I am keen to get this MR resolved.

pub fn add_inherit(&mut self) -> &mut SbgpAsIdentifier {
if let SbgpAsIdentifierOrInherit::List(ref l) = self.0 {
if !l.is_empty() {
panic!("cannot set extension to inherit, list allready contains elements");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
panic!("cannot set extension to inherit, list allready contains elements");
panic!("cannot set extension to inherit, list already contains elements");

Similar typos are throughout the panic strings

Choose a reason for hiding this comment

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

No problem, I will fix all these

@botovq
Copy link
Contributor

botovq commented Jul 1, 2024 via email

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.

7 participants