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 support for setting certificate on PeerConnection #1170

Merged
merged 2 commits into from
May 13, 2024

Conversation

xicilion
Copy link
Contributor

In Peer B of the WebRTC Direct protocol, a specific certificate and private key need to be specified in order for Peer B to maintain a fixed fingerprint.

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

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

I've no idea why at the moment but MbedTLS fails to parse the certificate in the test.

Comment on lines 92 to 93
optional<string> certPem;
optional<string> keyPem;
Copy link
Owner

Choose a reason for hiding this comment

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

You should make the configuration fields consistent with existing ones in WebSocketConfiguration and WebSocketServerConfiguration. The fields are called certificatePemFile, keyPemFile, and keyPemPass, they allow the user to pass certificate and key either as a file path or as a PEM string.

std::promise<certificate_ptr> cert;
cert.set_value(std::make_shared<Certificate>(Certificate::FromString(config.certPem.value(), config.keyPem.value())));
mCertificate = cert.get_future();
} else {
Copy link
Owner

Choose a reason for hiding this comment

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

You should create the certificate if both certificate and key are not specified, else throw an invalid_argument exception (like in WebSocketServer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get it.

@xicilion
Copy link
Contributor Author

I've no idea why at the moment but MbedTLS fails to parse the certificate in the test.

I am also confused, this certificate is created in openssl.

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

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

It looks good, thank you.

@paullouisageneau paullouisageneau changed the title Support for setting the cert and key on new PeerConnection. Support for setting the cert and key on new PeerConnection May 13, 2024
@paullouisageneau paullouisageneau changed the title Support for setting the cert and key on new PeerConnection Add support for setting certificate for PeerConnection May 13, 2024
@paullouisageneau paullouisageneau changed the title Add support for setting certificate for PeerConnection Add support for setting certificate on PeerConnection May 13, 2024
@paullouisageneau paullouisageneau merged commit 16c917f into paullouisageneau:master May 13, 2024
10 of 12 checks passed
@paullouisageneau
Copy link
Owner

Issue with MbedTLS parsing was preexisting, it is fixed by #1180

@xicilion
Copy link
Contributor Author

great!

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