Prepare 0.13.0 release with Rustls 0.23

[cpu]

Addresses upstream breaking API changes and updates associated Rustls dependencies to use the newest Rustls release.

Most notable for rustls-ffi are the changes in the rustls_acceptor_accept and rustls_accepted_into_connection
functions where a new rustls_accepted_alert out parameter is added.

This type wraps the upstream AcceptedAlert type and allows the caller to gather any to-be-written TLS data (most notably, TLS alerts) that was produced while trying to accept the client connection. The caller should handle the to-be-written alert bytes by calling rustls_accepted_alert_write_tls with the rustls_accepted_alert and a rustls_write_callback implementation to write the returned bytes to the connection before calling rustls_accepted_alert_free.

[cpu]

Investigate Windows cmake build failures

Something seems up with the cmake build tasks. They're both failing to link with unresolved external symbol errors:

rustls_ffi.lib(std-828e2398afa6ec92.std.37d137a6c0aa880e-cgu.0.rcgu.o) : error LNK2019: unresolved external symbol __imp_WakeByAddressSingle referenced in function _ZN4core3ptr56drop_in_place$LT$std..thread..Packet$LT$$LP$$RP$$GT$$GT$17h27aa9d331296562bE [D:\a\rustls-ffi\rustls-ffi\build\tests\client.vcxproj]
rustls_ffi.lib(std-828e2398afa6ec92.std.37d137a6c0aa880e-cgu.0.rcgu.o) : error LNK2019: unresolved external symbol __imp_WaitOnAddress referenced in function _ZN3std6thread4park17hc58846d9dce664faE [D:\a\rustls-ffi\rustls-ffi\build\tests\client.vcxproj]

[cpu]

I'm not sure why aws-lc-rs is still being built as a dependency of rustls with the no-default-features and feature opt-in we've done in this branch,

Ah! 💡 It's std: rustls/rustls#1818

[cpu]

Something seems up with the cmake build tasks.

Unrelated, but on my mind: #390 I'm not sure I understand why it makes sense to invest resources into the parallel cmake build infrastructure.

[cpu]

Something seems up with the cmake build tasks. They're both failing to link with unresolved external symbol errors:

Fixing this seems to require the addition of Synchronization.lib to the Cmake target_link_libraries invocations (fd92f13). I suspect this is from the addition of once_cell as a dependency upstream for the process default crypto provider.

I thought the CI static libs test was meant to catch this. I'm also not sure why the base Makefile approach doesn't require the update 🤔

[cpu]

I'm not sure why aws-lc-rs is still being built as a dependency of rustls with the no-default-features and feature opt-in we've done in this branch,

Ah! 💡 It's std: rustls/rustls#1818

I updated the git patch to pick up this change and was able to drop the CI changes adding nasm to the Windows build tasks.

[cpu]

Finish support for Acceptor error alert handling.

This is related to rustls/rustls#1811 and rustls/rustls#1792 - the previous API swallowed alerts generated during accept or into_connection. As implemented in this branch, we do the same thing: dropping any alerts on the floor. The upstream Rustls implementation now returns (Error, AcceptedAlert) in the error condition, and the caller must use AcceptedAlert::write to potentially write TLS alerts to the wire, allowing for better handling here.

I'm not sure the best way to represent this for the FFI boundary. One idea: the rustls_acceptor_accept and rustls_accepted_into_connection fn can gain two new out parameters, one *mut u8 for the data, and one *mut size_t for how much alert data was written. The wrapper would handle the error condition by writing from AcceptedAlert into the out buffer, and then returning a failure rustls_result. The caller would be responsible for copying the correct number of bytes from the out buffer to the socket, but only for the error condition. This seems awkward, but so is basically every C API... 😆

Any other ideas?

[jsha]

One idea: the rustls_acceptor_accept and rustls_accepted_into_connection fn can gain two new out parameters, one *mut u8 for the data, and one *mut size_t for how much alert data was written. The wrapper would handle the error condition by writing from AcceptedAlert into the out buffer, and then returning a failure rustls_result. The caller would be responsible for copying the correct number of bytes from the out buffer to the socket, but only for the error condition. This seems awkward, but so is basically every C API... 😆

I think an additional out parameter is the correct direction. But it needs to be an owned type with its own lifetime, so the C code can free it when done. If we ask the caller to provide a buffer as you're proposing, we run into problems like figuring out how big a buffer to allocate[1]. Since the Rust side is going to allocate a ChunkVecBuffer anyhow, we might as well use that.

So this looks like

• expose the AcceptedAlert type as *rustls_accepted_alert
• implement an accessor for the bytes inside it
• implement rustls_accepted_alert_free()
• add a second outparam for the two affected functions, documenting that at most one of the two outparams will be set, and that which one is set is fully determined by whether an error was returned.

[1]: But thinking about it a second time: can we put a small upper bound on the size of the alert bytes? If so, that makes the problem of the output buffer less annoying. We could tell people to allocate a small array on the stack and simply write to that, avoiding complications of malloc'ing and so on.

[cpu]

I think an additional out parameter is the correct direction. But it needs to be an owned type with its own lifetime

Aha, that does making arranging things easier 👍

Since the Rust side is going to allocate a ChunkVecBuffer anyhow, we might as well use that.

I ended up introducing a new crate-internal type on our side for this (AcceptedAlertBuffer) . The ChunkVecBuffer in the upstream AcceptedAlert isn't accessible. With the current API in 0.23 you have to call write to copy the data out.

So this looks like ...

Other than the caveat about needing our own Vec I think the plan you proposed and what I've implemented in-branch were pretty much in-sync 👍 Thanks!

[1]: But thinking about it a second time: can we put a small upper bound on the size of the alert bytes? If so, that makes the problem of the output buffer less annoying.

I thought about this a little bit but the AcceptedAlert's ChunkVecBuffer is the sendable_tls buffer from the connection's common data. It isn't scoped to alerts specifically, or guaranteed to be a specific number of alerts. It seemed fragile/dangerous to try and assume a safe static buffer size given that so I went with the other design hashed out in the comment thread.

[cpu]

I think this is ready for review now. (@ctz maybe you would be interested in giving it a once-over as well?)

[ctz]

rustls_accepted_alert as a solution looks pretty nice to me.

[cpu]

@jsha Are there any other changes you'd like to see in this branch?

[jsha]

Oops, I thought I had approved this. Thanks for the ping. Had one more small piece of feedback; with that addressed, consider this approved by me. Thanks!

[jsha]

Looks good!