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

expose DANE functions for SSL/SSL_CTX #2057

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

Conversation

wez
Copy link

@wez wez commented Oct 14, 2023

I'd like to try implementing DANE support in my tokio based program; making the functions available in the sys crate seems like the first logical step.

The docs at https://www.openssl.org/docs/man1.1.1/man3/SSL_dane_clear_flags.html indicate that DANE functionality has been available since version 1.1.0, so I've gated these to that version AFAICT.

@wez wez mentioned this pull request Oct 14, 2023
openssl/src/ssl/mod.rs Outdated Show resolved Hide resolved
openssl/src/ssl/mod.rs Outdated Show resolved Hide resolved
openssl/src/ssl/mod.rs Outdated Show resolved Hide resolved
openssl/src/ssl/mod.rs Outdated Show resolved Hide resolved
openssl/src/ssl/mod.rs Outdated Show resolved Hide resolved
@sfackler
Copy link
Owner

Can you also add some tests?

@wez
Copy link
Author

wez commented Oct 27, 2023

I'd love some feedback on the current state of this if you can find the time over this coming weekend!

@wez
Copy link
Author

wez commented Nov 3, 2023

Friendly ping! If there's something I need to tweak, please let me know!

@wez
Copy link
Author

wez commented Nov 13, 2023

@sfackler I'd appreciate some feedback on this, thanks!

@wez
Copy link
Author

wez commented Nov 20, 2023

Please let me know what I can do to help make progress here, thanks!

@wez
Copy link
Author

wez commented Nov 29, 2023

I'd love to get this merged, please let me know how I can help make that happen!

@wez wez force-pushed the dane branch 2 times, most recently from 583e74a to 94053bb Compare November 29, 2023 22:21
&mut self,
md: Option<crate::md::MdRef>,
mtype: DaneMatchType,
ord: u8,
Copy link
Owner

Choose a reason for hiding this comment

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

Are the ordinal values here supposed to just be raw integers between 0 and 255?

/// Requires OpenSSL 1.1.0 or newer.
#[corresponds(SSL_CTX_dane_set_flags)]
#[cfg(ossl110)]
pub fn set_no_dane_ee_namechecks(&mut self) {
Copy link
Owner

Choose a reason for hiding this comment

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

We should just provide dane_set_flags etc methods rather than separate ones per flag.

/// Requires OpenSSL 1.1.0 or newer.
#[corresponds(SSL_add1_host)]
#[cfg(ossl110)]
pub fn add1_host(&mut self, hostname: &str) -> Result<(), ErrorStack> {
Copy link
Owner

@sfackler sfackler Dec 4, 2023

Choose a reason for hiding this comment

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

The 1 shouldn't be in the Rust method name - it indicates the lack of ownership transfer in the C API which isn't relevant for a Rust caller.

))
}?;

Ok(usable > 0)
Copy link
Owner

Choose a reason for hiding this comment

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

Is an error stack not returned in the 0 case? I think this might be better off just returning Result<(), ErrorStack>.

The docs at https://www.openssl.org/docs/man1.1.1/man3/SSL_dane_clear_flags.html
indicate that DANE functionality has been available since version 1.1.0
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.

2 participants