Skip to content

Conversation

@jrose-signal
Copy link
Contributor

Had occasion to want these, hopefully relatively uncontroversial. I did see that there are two different ways of adapting the "measure once, output once" APIs: SslSession methods follow this approach, using an &mut [u8] argument and special-casing the empty case (though apparently that's done within BoringSSL itself); EcPointRef::to_bytes, by contrast, just calls the underlying BoringSSL API twice so it can return a Vec<u8>. Since the caller must already know something about the kind of key to expect success, I figured it was worth providing the more flexible slice-based API.

@kornelski
Copy link
Collaborator

This is a useful functionality, but the API is very C-like. Getting the length with &mut [] looks odd:

buffer.resize(pk.raw_public_key(&mut [])?, 0);
let len = pk.raw_public_key(&mut buffer)?;
let key = &buffer[..len];

Since we don't already have a great consistency, I suggest improving the approach further.

If you add raw_public_key_len(), and make the raw_public_key() require a non-empty buffer and return the slice trimmed to the correct length:

fn raw_public_key<'buf>(&self, buf: &'buf mut [u8]) -> Result<&'buf [u8], ErrorStack> {Ok(&buf[..size])
}

then the API becomes nicer to use:

let key = pk.raw_public_key(&mut buffer)?; // assuming we know the size

and:

buffer.resize(pk.raw_public_key_len()?, 0);
let key = pk.raw_public_key(&mut buffer)?;

I've explored different variations like Result<Either<&[u8], usize>, ErrorStack> or Result<&[u8], Result<usize, ErrorStack>>, or taking a closure that configures a buffer when needed, but having a separate raw_public_key_len method ends up being the simplest.

@jrose-signal
Copy link
Contributor Author

Done! I didn't include an explicit check for a non-empty buffer because BoringSSL should already do that—an empty buffer is just a special case of a too-short buffer.

@jrose-signal jrose-signal requested a review from nox June 10, 2025 17:39
@nox
Copy link
Collaborator

nox commented Jun 11, 2025

Now I'm wondering, should the input be of type &mut [MaybeUninit<u8>]? Or do we not care about that? Any opinion @kornelski?

@kornelski
Copy link
Collaborator

Keys are small. MaybeUninit would be an overkill.

@kornelski kornelski merged commit b01510d into cloudflare:master Jun 13, 2025
23 checks passed
@kornelski kornelski mentioned this pull request Jun 13, 2025
8 tasks
@jrose-signal jrose-signal deleted the jrose/pkey-raw-key-accessors branch June 13, 2025 16:39
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.

4 participants