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 other name support #1915

Merged
merged 1 commit into from
May 26, 2023
Merged

add other name support #1915

merged 1 commit into from
May 26, 2023

Conversation

huettner94
Copy link
Contributor

the issue with other name SANs is that they can contain arbitary data. As we can no longer use the old method for other_name for security reasons the easiest way now is to do individual implementations per datatype.

We start with strings for now, but it should be quite easy to expand upon that.

Closes: #1911

@huettner94
Copy link
Contributor Author

note: i did not test with boringssl i hope the ci does that

@huettner94 huettner94 force-pushed the feat_other_name branch 2 times, most recently from 58a5e90 to 8f99f0f Compare May 1, 2023 20:09
Copy link
Collaborator

@alex alex 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 diving into this.

pub fn other_name(&mut self, _other_name: &str) -> &mut SubjectAlternativeName {
unimplemented!(
"This has not yet been adapted for the new internals. File a bug if you need this."
);
}

/// Sets the `otherName` flag to a ia5string value.
pub fn other_name_string(&mut self, oid: &str, content: &str) -> &mut SubjectAlternativeName {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we want to use more precise types here: oid should be an Asn1ObjectIdentifier type, and contents should be &[u8] since it's DER.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand that idea, but i would also propose to support a more "userfriendly" way of doing things.

When the user can add arbitrary data as &[u8] it is not that easy to use that function anymore (and we need to define it as unsafe, since openssl has certain assumptions about the data).

If i know that field is a IA5STRING, then it would be ideal from my perspective if this library cares about the needed unsafe magic and i can just use the "easy" methods.

Would it be ok for you to add two versions? One that allows arbitrary (but unsafe) data, and one that is restricted to Strings?

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO limiting to strings doesn't very much sense, OtherName is a very general purpose specification, and we should have an API that's similarly broad. Why do you think unsafe would be required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats why i would allow both. To get the combination of ease of use and all the features you might need.

I see unsafe as required as the data would then be passed to GENERAL_NAME_set0_othername. In there the openssl lib expects (if i understood it correctly) certain data formats (e.g. during deserialization). At least the last parameter must be a ASN1_TYPE as otherwise you will get a SegmentationFault (which is a clear sign to me that we need unsafe here, or guarantee that we do not need it like in the string case)

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the public API takes &[u8], a sequence of DER bytes, then it will be the responsibility of rust-openssl to have the unsafe code to turn that into an ASN1_TYPE, we can do that with d2i_ASN1_TYPE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification.

I have now added other_name2 that will take a provided &[u8] and try to convert it to an ASN1_TYPE.

For ease of use i have adapted the other_name_ia5string method, but if you want to get rid of it that is also fine for me.

Comment on lines 2059 to 2060
let s = cvt_p(ffi::ASN1_STRING_type_new(Asn1Type::IA5STRING.as_raw()))?;
ffi::ASN1_STRING_set(s, value.as_ptr().cast(), value.len().try_into().unwrap());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't right, the value of an OtherName is not necessarily an IA5String, we need to handle arbitrary DER TLVs in the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator

@alex alex left a comment

Choose a reason for hiding this comment

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

Getting close! Can you please add a test case or two to this.

Comment on lines 531 to 525
) -> Result<&mut SubjectAlternativeName, ErrorStack> {
let typ: *mut ASN1_TYPE;
unsafe {
ffi::init();

typ = cvt_p(ffi::d2i_ASN1_TYPE(
ptr::null_mut(),
&mut content.as_ptr().cast(),
content.len().try_into().unwrap(),
))?;
}
self.items
.push(RustGeneralName::OtherName(oid.to_string(), typ));
Ok(self)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would like @sfackler's take on this, specifically whether we should have this return a Result, or if it should be consistent with the other ones and just return &mut SAN and then do the error handling lazily.

/// If you want to add just a ia5string use `other_name_ia5string`
pub fn other_name2(
&mut self,
oid: &str,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 547 to 552
/// Sets the `otherName` flag for ia5strings.
pub fn other_name_ia5string(
&mut self,
oid: &str,
content: &str,
) -> &mut SubjectAlternativeName {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer not to expose this -- the right way to do this is a separate, orthogonal API that makes it easy to DER serialize a https://docs.rs/openssl/latest/openssl/asn1/struct.Asn1String.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, added that api in a separate commit

let gn = cvt_p(ffi::GENERAL_NAME_new())?;
(*gn).type_ = ffi::GEN_OTHERNAME;

ffi::GENERAL_NAME_set0_othername(gn, oid.as_ptr().cast(), value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can technically fail and we should handle that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@huettner94
Copy link
Contributor Author

i will add tests soon(tm)

Copy link
Collaborator

@alex alex left a comment

Choose a reason for hiding this comment

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

A few small comments, this is very close. Thanks for your work.

&mut self,
oid: Asn1Object,
content: &[u8],
) -> Result<&mut SubjectAlternativeName, ErrorStack> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sfackler do you have an opinion on this returning a Result, vs. returning &mut SAN and doing the fallible ops in build?

Copy link
Owner

Choose a reason for hiding this comment

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

Given that we're deferring the other fallible work to build currently, I'd have this do the same I think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, so in that case this should just return the builder, and we can conver to ASN1_TYPE in build and I think that will fix the other soundness problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

Comment on lines 2060 to 2064
cvt(ffi::GENERAL_NAME_set0_othername(
gn,
oid.as_ptr().cast(),
value,
))?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this fails then gn is leaked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 433 to 456
impl Asn1String {
/// Converts a `&str` to an `Asn1String`.
pub fn from_str(data: &str) -> Result<Self, ErrorStack> {
unsafe {
let s = cvt_p(ffi::ASN1_STRING_type_new(Asn1Type::IA5STRING.as_raw()))?;
ffi::ASN1_STRING_set(s, data.as_ptr().cast(), data.len().try_into().unwrap());
Ok(Self::from_ptr(s))
}
}

/// Converts the Asn1String to the ASN.1 representation with a IA5String tag
pub fn as_asn1type_der(&self) -> Result<Vec<u8>, ErrorStack> {
unsafe {
let typ = cvt_p(ffi::ASN1_TYPE_new()).unwrap();
ffi::ASN1_TYPE_set(typ, ffi::V_ASN1_IA5STRING, self.0.cast());

let len = cvt(ffi::i2d_ASN1_TYPE(typ, ptr::null_mut()))?;
let mut buf = vec![0; len as usize];
crate::cvt(ffi::i2d_ASN1_TYPE(typ, &mut buf.as_mut_ptr()))?;
Ok(buf)
}
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do this in a separate PR? It should be orthagonal to the rest of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, will open one soon

@@ -705,6 +729,12 @@ impl Asn1Object {
}
}

impl Clone for Asn1Object {
fn clone(&self) -> Self {
unsafe { Asn1Object::from_ptr(cvt_p(ffi::OBJ_dup(self.as_ptr())).unwrap()) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of this you can do clone = OBJ_dup; in the foreign_type_and_impl_send_sync macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, done

Comment on lines 514 to 515
/// Not currently actually supported, always panics. Please use other_name2 or other_name_ia5string
#[deprecated = "other_name is deprecated and always panics. Please use other_name2 or other_name_ia5string."]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Not currently actually supported, always panics. Please use other_name2 or other_name_ia5string
#[deprecated = "other_name is deprecated and always panics. Please use other_name2 or other_name_ia5string."]
/// Not currently actually supported, always panics. Please use other_name2
#[deprecated = "other_name is deprecated and always panics. Please use other_name2."]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks fixed

pub fn other_name(&mut self, _other_name: &str) -> &mut SubjectAlternativeName {
unimplemented!(
"This has not yet been adapted for the new internals. File a bug if you need this."
"This has not yet been adapted for the new internals. Use other_name2 or other_name_ia5string."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"This has not yet been adapted for the new internals. Use other_name2 or other_name_ia5string."
"This has not yet been adapted for the new internals. Use other_name2."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks fixed

@huettner94
Copy link
Contributor Author

i added a small testcase. is that sufficient or would you like a larger one

huettner94 added a commit to huettner94/dtn that referenced this pull request May 17, 2023
@huettner94 huettner94 force-pushed the feat_other_name branch 2 times, most recently from 8ae9cd1 to b5a6abc Compare May 17, 2023 21:25
@@ -506,12 +511,34 @@ impl SubjectAlternativeName {

/// Sets the `otherName` flag.
///
/// Not currently actually supported, always panics.
#[deprecated = "other_name is deprecated and always panics. Please file a bug if you have a use case for this."]
/// Not currently actually supported, always panics. Please use other_name2 or other_name_ia5string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Not currently actually supported, always panics. Please use other_name2 or other_name_ia5string
/// Not currently actually supported, always panics. Please use other_name2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks fixed

Comment on lines 2068 to 2078
if let Err(e) = cvt(ffi::GENERAL_NAME_set0_othername(
gn,
oid.as_ptr().cast(),
value,
)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately this isn't sound -- because build takes &self you can call it multiple times, in which case the value ptr will be reused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

handled as already commented somewhere else

the issue with other name SANs is that they can contain arbitary data.
As we can no longer use the old method for other_name for security
reasons we now add `other_name2` as an alternative.
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.

support SubjectAlternativeName::other_name again
3 participants