Remove X509Extension::add_alias

[davidben]

It is impossible to use this function without introducing a security vulnerability in your program. It should simply be removed. See https://crbug.com/boringssl/590 for details.

As noted in that bug, not only is this always function incorrect to call, it's also pointless. I suspect rust-openssl added it because X509Extension::new was based on the string-based system, which was itself a security vulnerability in the library. (RUSTSEC-2023-0023 and #1854) Had rust-openssl provided the correct extension construction API (i.e. the underlying DER bytes, not the ad-hoc, OpenSSL-specific, and ambiguous text format), add_alias would not have been needed to add custom extensions.

(Strictly speaking, it's already not needed because of the ASN1:whatver mode of the mini-language, but you all should stop using the mini-language because it's a security bug. It's particularly unreasonable when purporting to provide APIs in a safe language. 😄 #1854 fixed the worst of it, but it just added a warning on X509Extension::new without providing a replacement.)

Once you've removed that, I think you'll also have removed the only use case for OBJ_create and should remove that too. OBJ_create has similar problems with global state. To that end, Nid::create is missing a warning about how unsafe it is... I'll file a bug there too. See BoringSSL's warning.

I hope to remove these unsafe functions from BoringSSL altogether, so if I've cleared the other blockers before rust-openssl's bugs are fixed, you all may need to ifdef this API out of your BoringSSL port.

CC @alex and @reaperhulk, since we've been talking about various security problems caused by rust-openssl binding the wrong APIs.

[alex]

I think the TODO list here is roughly:

• Add a new X509Extension::new_from_der method that takes an OID, critical, and DER bytes. This lets you encode arbitrary extensions.
• Deprecate (with intent to remove) X509Extension::new, X509Extension::new_nid, and X509Extension::add_alias - referring folks to either the dedicated extension types or new_from_der
• Consider adding an extensible Extensions API like the one described here Add issuer_name and reason_code to X509RevokedRef #1847 (comment)

@sfackler thoughts? If this sounds good, I can start the PR train.

[sfackler]

Yep, that seems right to me.

[sfackler]

Once we add the new extensions API there are a fair number of ad-hoc methods like X509Ref::subject_alt_names that we can deprecate as well.

[davidben]

As long as the extensible extensions API doesn't require process-global knowledge like the OpenSSL ones do, I don't have a correctness objection. (I.e. no NIDs, and two different libraries can have different preferences on how to process the same extension.) But, based on my experience with X.509 itself, I think an extensible API is not the way to go.

"Ad-hoc" methods good here. They are easy to understand and easy to call. I want the SAN list? Okay, I look for whether there's a method that gives me the SAN list. It also gives you more room to design good APIs independent of encoding quirks. The fact that some things are encoded as extensions and some aren't is a historical quirk of how the format evolved. Groupings into extensions can also be quirky... requireExplicitPolicy and inhibitPolicyMapping are grouped into one extension, but inhibitAnyPolicy is separate. Basic constraints combines two different constraints together.

And if callers want to implement something you don't support, that is also straightforward: provide an API to lookup an arbitrary extension and return actual extension field. Indeed this is would match the spirit of having 1:1 APIs with X.509. (The bug cites 1:1 APIs with OpenSSL, but I'll note that style has gotten you into trouble already.)

Indeed, we can look to other X.509 APIs. Go just maps known extensions to fields, but makes the other extensions available as bytes for perusal. Java likewise has accessors for known fields, and then if you need something beyond that, you can query the extension list.

Generic APIs make sense if you expect people to call them generically, or if they're a good way to express some useful abstraction. There's no reason to call your extensions API generically. Indeed OpenSSL is the example that proves the rule. You always know the extension you're trying to process because you cannot do anything useful with an extension you don't know about.

As for abstractions, if someone wants to process an extension that rust-openssl doesn't know about, they need to:

• Look up the extension by OID and see if it's there (common)
• Take the bytes they looked up and parsed them (caller-specific)

Sure, you can model this by taking the parse callback as a trait, but all you're really abstracting is "call one function after another". That's something the language already can do. Just provide a lookup function with bytes, and let the caller call it. The extra complexity buys nothing. If anything, it's more verbose because you need to wire up a trait before you can call the function.

X.509 extensions are simply fields. We call them "extensions" because when two different implementations talk to each other, we need a model for evolving the structure. But that's about interchange, not API design.