Use X509_PURPOSE_get_id instead of struct access

[davidben]

The accessor was added at the same version as the struct, so better to just use it. As with X509_PURPOSE_get_by_sname, it was const-corrected later on.

[alex]

@sfackler are there backwards compataibility issues with making teh struct opaque?

Otherwise the PR looks good to me.

[sfackler]

Yeah, we should really keep the struct definition as-is to preserve back compat. If you want, we could make it opaque for future open/boring/libre releases though.

[davidben]

We will never, ever support using the handwritten bindings with BoringSSL because of memory safety, so I can just put it back. Though you all really should rethink the stability expectations there.

[botovq]

On Sat, Dec 02, 2023 at 01:53:35PM -0800, Steven Fackler wrote: Yeah, we should really keep the struct definition as-is to preserve back compat. If you want, we could make it opaque for future open/boring/libre releases though.

I think this is the least that should be done. The xstandard[] table containing the standard purposes is static but not const in all three libraries (I plan on changing that in libre). X509_PURPOSE_get0() returns a pointer to it (or worse application configured global mutable state if someone called X509_PURPOSE_add()), so X509PurposeRef::from_idx() exposes a pointer to global mutable state to Rust.

[davidben]

X509_PURPOSE_get0() returns a pointer to it (or worse application configured global mutable state if someone called X509_PURPOSE_add()), so X509PurposeRef::from_idx() exposes a pointer to global mutable state to Rust.

Oh, the entire X509PurposeRef API should be removed. It makes no sense in the first place. This is just the usual issue where binding APIs without thinking through use cases leads to problems.

The only thing one can do with X509_PURPOSE_get_by_sname is to then chain it with X509_PURPOSE_get0, followed by X509_PURPOSE_get_id, and then configure it on an X509_STORE. Not only that, short of X509_PURPOSE_add (which is globally not thread-safe, so it is impossible to expose in Rust safety), the only possible outputs of X509_PURPOSE_get_by_sname are the predefined constants anyway.

That means the only APIs here that made sense were the built-in X509PurposeId constants and set_purpose. Everything else was just a mistake and should be removed.

[davidben]

I plan on changing that in libre

BoringSSL will be doing that too. Note that, similar to #2113, it may run into a design flaw in foreign-types. From talking with @alex, it sounds like that crate uses exclusively mut pointers and doesn't honor const in C? This is, of course, a pretty serious safety issue.

I think I'll also just remove the APIs that X509PurposeRef relies on, since it's now clear rust-openssl's use is invalid.