Icing/enums #56

icing · 2021-03-08T08:52:39Z

Not sure how we want to merge all this. I think it is time to merge something of our changes into main and have a common base where to add future changes to.

…hello inspection.

Adjusted the ClientHello definitions, Optional checks and workings to the code as written in https://github.com/abetterinternet/crustls/compare/server-cert-verifier. All errors are mine.

Adding the u16 values of the SignatureScheme values announced by the client hello to the callback structure. Adding a function to retrieve a name for the scheme, insofar as rustls supports it.

- Added a base.rs for structs providing strings, string arrays and short arrays to C callback functions. - Extended rustls_client_hello, adapted the prototype and added documentation. - Made a section about EXPERIMENTAL features in the README.md

We now have four types that map nicely to Rust types: - rustls_str -> &str or &String - rustls_slice_bytes -> &[u8] or &Vec<u8> - rustls_vec_str -> &[&str] or &Vec<&str> - rustls_vec_slice_bytes -> &[&[u8]] or &Vec<Vec<u8>> I also added lifetime parameters for these types, along with PhantomData to refer to those lifetime parameters. This lets us take slightly more advantage of Rust's safety tools by ensuring we don't allow any of these types to outlive the things they refer to.

Adding `rustls_bytes` as type similar to `rustls_string` to make clear what sort of data is passed. Renaming rustls_string_vec consequently to rustls_bytes_vec, as ALPN values are bytes and not necessarily proper utf-8.

A rustls `CertifiedKey` can be constructed, given chain and key PEM data. Such a key can be returned by the client_hello callback or configured at the builder for use in the created session. The builder can be provided with a list of certified_keys and the session will chose the first one from the list that is compatible with the signatures the client provided in its hello.

… client_hello_callback_update_base

…te_base Update base.rs.

the lifetime of allocated array data for the public struct of similar name.

The new `rustls_server_config_builder_from_config` allows the creation of a builder from an existing config. This clones the config and leaves the passed config unchanged, as constness implies. The new builder inherits this way all existing configuration values and servers can alter these before creating a session with specific needs. Example: this is used in mod_tls to create a session with exactly the ALPN protocol value that was chosen based on the client hello's list.

The current design of crustls by explicitly NULing write buffers in a tight loop was disabled and commented with links to the relevant discussions in the Rust language and alternatives.

…ed ciphers. * a server session can be asked about negotiated TLS protocol version and ciphersuite * these are given as u16 values as defined in the protocol RFCs and IANA registries * a name conversion for ciphersuite is available * an application can iterate over all supported ciphersuite of rustls, inspecting id and name.

There are names defined for TLS ciphers suites that are used in various configuration directives. For an example see: <https://wiki.mozilla.org/Security/Server_Side_TLS>. I could not find a way in rustls to obtain this name (but I have overlooked things before), so I introduced the 'correct' names in the enums and let users of the API retrieve them.

jsha

I appear to have made two mistakes with this PR; my apologies. The first mistake is that I appear to have auto-closed it when I merged the rslice branch. Not sure why that happened. But since the rslice branch is also deleted, GitHub won't allow me to reopen it. Please go ahead and reopen against main.

The other mistake was that I wrote up some feedback yesterday but neglected to send it! Sending this now. I think it's still worthwhile having some conversation about implementation while we also discuss how much configuration to expose, though I also understand if you want to postpone investment on this side of the code until that is settled.

jsha · 2021-03-09T00:08:21Z

src/cipher.rs

+    ))
+}
+
+struct CiperSuites<'a> {


It looks like this struct is always built, used to look something up, and then thrown away. Wouldn't it be simpler and more efficient to, for example, implement rustls_cipher_suite_is_supported as rustls::ALL_CIPHERSUITES.iter().contains(suite_num)? Another alternative would be to use OnceCell to initialize a single global CipherSuites object, but I think ALL_CIPHERSUITES will always be short enough that that's not needed.

jsha · 2021-03-09T00:24:21Z

src/cipher.rs

+/// The complete chain of certificates plus private key for
+/// being certified against someones list of trust anchors (commonly
+/// called root store). Corresponds to `CertifiedKey` in the Rust API.
+pub struct rustls_cipher_certified_key {


Suggested change

pub struct rustls_cipher_certified_key {

pub struct rustls_certified_key {

I think we don't need cipher in the name because this doesn't do anything specific to ciphersuites. In general my naming guideline has been: If a struct's Rust name is unambiguous, just prefix it with rustls_ (and snake_case it).

Changed already.

jsha · 2021-03-09T00:29:16Z

src/cipher.rs

+#[no_mangle]
+pub extern "C" fn rustls_cipher_certified_key_build(


Needs a doccomment. In particular it should explain what format the input arguments should be in.

jsha · 2021-03-09T00:32:12Z

src/cipher.rs

+
+/// "Free" a certified_key previously returned from
+/// rustls_cipher_certified_key_build. Since certified_key is actually an
+/// atomically reference-counted pointer, extant certified_key may still


Suggested change

/// atomically reference-counted pointer, extant certified_key may still

/// atomically reference-counted pointer, extant certified_keys may still

jsha · 2021-03-09T01:33:28Z

src/cipher.rs

+        let len: usize = min(write_buf.len() - 1, name.len());
+        write_buf[..len].copy_from_slice(&name.as_bytes()[..len]);


I think we have a couple of choices here:

return an error if the stringified name is longer than the output buffer, or

document the maximum size of output we will emit, and truncate if the output buffer is too small (i.e. what the code currently does).

I don't have a strong opinion, but if we proceed with (2) we don't need to return rustls_result, because the only error case is "some inputs are null," and we specify in the README that if the only reason for returning a rustls_result is for null checking, we don't necessarily return one.

Or, a third choice:

Make each name a &str with static lifetime (i.e. the whole program). Then we can give the user a pointer to that &str and tell them they don't have to worry about freeing it because it will live forever.

jsha · 2021-03-09T01:39:01Z

src/cipher.rs

+        if strong_count < 1 {
+            eprintln!(
+                "rustls_cipher_certified_key_free: invariant failed: arc.strong_count was < 1: {}. \
+                You must not free the same certified_key multiple times.",
+                strong_count
+            );
+        }


I removed this strong_count check from client.rs in #32, but I see I left a copy in server.rs. I'll remove that. Could you remove this check too?

jsha · 2021-03-09T02:33:17Z

src/enums.rs

+pub extern "C" fn rustls_supported_ciphersuite_iter(
+    callback: rustls_supported_ciphersuite_callback,
+    userdata: rustls_supported_ciphersuite_userdata,


The iterator-and-callback approach seems like a lot of complexity for a fairly simple task. Can we do something like:

rustls_all_ciphersuites_len() -> size_t rustls_all_ciphersuites_get(i: size_t) -> *const rustls_supported_ciphersuite

jsha · 2021-03-09T02:48:17Z

src/enums.rs

+/// Get the 'standard' name for a supported cipher suite. See
+/// <https://wiki.mozilla.org/Security/Server_Side_TLS> as an example
+/// for definitions.


I think these alternate names come from OpenSSL, right? E.g. ECDHE-ECDSA-AES128-GCM-SHA256 for 0xC0,0x2B per OpenSSL instead of TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 per IANA.

I'd prefer not to support OpenSSL-style names. I can see how they make things like the Mozilla TLS config generator easier to map onto mod_tls, but I think that's not super important.

rustls takes a very opinionated approach to ciphersuites and offers only a small set that are thought to be secure. I think in 99.9% of cases, the user will not need to customize ciphersuites with mod_tls, and will not need to make reference to the Mozilla config generator - things should just work in a high security mode out of the box.

jsha · 2021-03-09T03:19:28Z

src/enums.rs

+        for cipher in rustls::ALL_CIPHERSUITES.iter() {
+            let name = format!("{:?}", cipher);
+            let s: &str = &name;
+            let rs: rustls_str = s.try_into().ok().unwrap();


We shouldn't use unwrap because it panics.

jsha · 2021-03-09T03:24:04Z

src/enums.rs

+pub(crate) fn rustls_signature_schemes_to_u16s(schemes: &[rustls::SignatureScheme]) -> Vec<u16> {
+    let mut mapped_schemes: Vec<u16> = Vec::new();
+    for s in schemes {
+        mapped_schemes.push(s.get_u16());
+    }
+    mapped_schemes
+}


Since this is not exposed to C (it's not extern "C"), it should use Rust-style capitalization. Also, I think this can be expressed succinctly like:

let mapped_schemes: Vec<u16> = schemes.iter().map(|s| s.get_u16()).collect();

In my opinion that's succinct and idiomatic enough that it doesn't need to have its own function but can just be written at the call site.

icing · 2021-03-10T09:32:30Z

Let's leave this closed. I will not start a new PR on this until we know how #50 will proceed. It does not make sense to develop any of this on main and review this without the use cases in server.rs being apparent and part of the discussion.

Stefan Eissing and others added 26 commits February 16, 2021 16:39

Experimental and probably horrible C callback implemented for client …

464d5d4

…hello inspection.

ClientHello callback modelled after ClientVerifier work.

2c16b04

Adjusted the ClientHello definitions, Optional checks and workings to the code as written in https://github.com/abetterinternet/crustls/compare/server-cert-verifier. All errors are mine.

Using size_t instead of uint to return sni_name_len.

6e10985

Adding client hello signature schemes to the callback struct.

60fc602

Adding the u16 values of the SignatureScheme values announced by the client hello to the callback structure. Adding a function to retrieve a name for the scheme, insofar as rustls supports it.

Implementation of resolved issues from review. More to follow.

5c6d1ea

Fixing typos, typedefs and signatures after review.

0ce34c9

Merge branch 'main' into client_hello_callback

31f1d2c

Adding rustls_bytes and slight renames of base methods.

75aaa1e

Adding `rustls_bytes` as type similar to `rustls_string` to make clear what sort of data is passed. Renaming rustls_string_vec consequently to rustls_bytes_vec, as ALPN values are bytes and not necessarily proper utf-8.

Merge branch 'client_hello_callback' of github.com:icing/crustls into…

b3f7fd6

… client_hello_callback_update_base

Merge pull request #1 from abetterinternet/client_hello_callback_upda…

24731ef

…te_base Update base.rs.

Adding rustls_vev_str_keeper and rustls_vec_slice_bytes_keeper to manage

cebba41

the lifetime of allocated array data for the public struct of similar name.

Renames after review by @jsha.

eee4118

write buffer initializdation skipped.

ecd74af

The current design of crustls by explicitly NULing write buffers in a tight loop was disabled and commented with links to the relevant discussions in the Rust language and alternatives.

Add rustls_str, rustls_slice_bytes, and friends

fa4c19b

Merge remote-tracking branch 'icing/client_hello_callback' into merged

c09b7f1

Update to use rslice.

14e31f0

Merged client_hello_callback_updates branch from jsha.

fb102fb

Merge remote-tracking branch 'upstream/rslice' into icing/main

df81966

interim

b26e486

tmp

cb9c476

jsha mentioned this pull request Mar 9, 2021

Ciphersuite selection philosophy #58

Open

jsha closed this Mar 9, 2021

jsha deleted the branch rustls:rslice March 9, 2021 17:24

jsha reviewed Mar 10, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Icing/enums #56

Icing/enums #56

Uh oh!

icing commented Mar 8, 2021

Uh oh!

jsha left a comment

Uh oh!

jsha Mar 9, 2021

Uh oh!

jsha Mar 9, 2021

Uh oh!

icing Mar 10, 2021

Uh oh!

jsha Mar 9, 2021

Uh oh!

jsha Mar 9, 2021

Uh oh!

jsha Mar 9, 2021

Uh oh!

jsha Mar 9, 2021

Uh oh!

jsha Mar 9, 2021

Uh oh!

jsha Mar 9, 2021

Uh oh!

jsha Mar 9, 2021

Uh oh!

jsha Mar 9, 2021

Uh oh!

icing commented Mar 10, 2021 •

edited

Loading

Uh oh!

Uh oh!

	pub struct rustls_cipher_certified_key {
	pub struct rustls_certified_key {

		#[no_mangle]
		pub extern "C" fn rustls_cipher_certified_key_build(

	/// atomically reference-counted pointer, extant certified_key may still
	/// atomically reference-counted pointer, extant certified_keys may still

		let len: usize = min(write_buf.len() - 1, name.len());
		write_buf[..len].copy_from_slice(&name.as_bytes()[..len]);

Icing/enums #56

Icing/enums #56

Uh oh!

Conversation

icing commented Mar 8, 2021

Uh oh!

jsha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icing commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

icing commented Mar 10, 2021 •

edited

Loading