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

Remove OpenSSL dependencies #12411

Merged
merged 2 commits into from
May 23, 2022

Conversation

LKozlowski
Copy link
Contributor

@LKozlowski LKozlowski commented May 4, 2022

This PR completely removes OpenSSL dependency from teleport binary.

It combines two things:

  1. use of rdp-rs library without OpenSSL dependency (it is using rustls)
  2. [draft] Use RustCrypto/RSA to sign smart card challenge, not OpenSSL #8914

Also, I've added docs sections for setting up certificate templates for RDP connection

@LKozlowski LKozlowski self-assigned this May 4, 2022
@zmb3 zmb3 requested review from xacrimon and zmb3 May 4, 2022 15:00
lib/srv/desktop/rdp/rdpclient/Cargo.toml Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
lib/srv/desktop/rdp/rdpclient/Cargo.toml Outdated Show resolved Hide resolved
@LKozlowski LKozlowski force-pushed the lkozlowski/remove-openssl-with-rdp-without-openssl branch 2 times, most recently from 6f8d20c to 89f6f33 Compare May 5, 2022 12:53
@LKozlowski LKozlowski marked this pull request as ready for review May 5, 2022 13:33
@LKozlowski LKozlowski force-pushed the lkozlowski/remove-openssl-with-rdp-without-openssl branch from 3753d9f to dea3560 Compare May 5, 2022 13:37
Copy link
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me, assuming the functionality was manually tested. Thanks for doing this work!

This PR completely removes OpenSSL dependency from Teleport's binaries.

Sadly, this isn't entirely true. libfido2 pulls libcrypto into our tsh builds, but I suppose it is true now for RDP. Please edit the description to mention that this is related to RPD, it may give the wrong impression to people otherwise.

docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Show resolved Hide resolved
lib/service/service.go Outdated Show resolved Hide resolved
lib/service/service.go Outdated Show resolved Hide resolved
@LKozlowski LKozlowski force-pushed the lkozlowski/remove-openssl-with-rdp-without-openssl branch from 06c0978 to ff6a870 Compare May 9, 2022 14:50
@LKozlowski LKozlowski requested a review from codingllama May 9, 2022 15:08
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
lib/service/service.go Outdated Show resolved Hide resolved
@LKozlowski LKozlowski force-pushed the lkozlowski/remove-openssl-with-rdp-without-openssl branch 2 times, most recently from 1a69ce5 to 6f29c97 Compare May 9, 2022 19:58
@zmb3 zmb3 added the release-notes A reminder label to into the release notes of the Teleport Release. label May 9, 2022
@LKozlowski
Copy link
Contributor Author

@codingllama I've reverted that indentation and slightly changed one item in the list to remove all the docs lint warnings. Also, now it is exactly as other lists and texts in this file, so I'm assuming everything will render correctly.

@@ -314,6 +314,62 @@ Computer Configuration > Policies > Administrative Templates > Windows Component
![Disable Require](../../img/desktop-access/disable.png)
</Figure>

### Configure certificate for RDP connections
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is the final subsection of "Step 3/6: Configure a GPO to allow Teleport connections". I'm not sure that's the intention, though, since the second paragraph begins with "In this step we'll...".

If this is the beginning of a new step, we'll need to promote this section heading to an H2 (using ## instead of ###), add "Step n/d:" to the title, and renumber the rest of the H2 section headings (e.g., this would be "Step 4/7").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about whether I should add it as a separate step or add it to the existing one as it's related to it. After I gave it some thought and re-read it again, I agree that it make more sense to make it a separate optional step.

docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
@LKozlowski LKozlowski requested a review from ptgott May 16, 2022 08:05
Copy link
Contributor

@ptgott ptgott left a comment

Choose a reason for hiding this comment

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

Approved with some very minor suggestions

docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
docs/pages/desktop-access/getting-started.mdx Outdated Show resolved Hide resolved
@LKozlowski LKozlowski force-pushed the lkozlowski/remove-openssl-with-rdp-without-openssl branch from 874b2e6 to ea39c88 Compare May 17, 2022 10:27
@LKozlowski LKozlowski enabled auto-merge (squash) May 19, 2022 09:23
@LKozlowski LKozlowski disabled auto-merge May 19, 2022 09:23
zmb3 and others added 2 commits May 23, 2022 11:04
RustCrypto is preferred, as it's a pure-Rust implementation, which
simplifies cross compilation for us and prevents us from needing to
pull in all of OpenSSL.

We originally thought that OpenSSL would be required here as RustCrypto
didn't appear to support RSA decryption without padding, but that
turned out to be false.
@LKozlowski LKozlowski force-pushed the lkozlowski/remove-openssl-with-rdp-without-openssl branch from 7637b33 to 55ed0c2 Compare May 23, 2022 09:05
@LKozlowski LKozlowski merged commit 59004b2 into master May 23, 2022
zmb3 added a commit that referenced this pull request May 27, 2022
* Bump rdp-rs (#11768)

I had made some changes to rdp-rs to expose new flags for tweaking
some performance characteristics, but never got around to pulling
in the update here.

Fix this now so as not to block upcoming work the team is doing
on rdp-rs.

* Use RustCrypto/RSA instead of OpenSSL

RustCrypto is preferred, as it's a pure-Rust implementation, which
simplifies cross compilation for us and prevents us from needing to
pull in all of OpenSSL.

We originally thought that OpenSSL would be required here as RustCrypto
didn't appear to support RSA decryption without padding, but that
turned out to be false.

* Remove OpenSSL dependency from the rust RDP client

Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com>
Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>
LKozlowski pushed a commit that referenced this pull request Jun 7, 2022
RustCrypto is preferred, as it's a pure-Rust implementation, which
simplifies cross compilation for us and prevents us from needing to
pull in all of OpenSSL.

We originally thought that OpenSSL would be required here as RustCrypto
didn't appear to support RSA decryption without padding, but that
turned out to be false.
LKozlowski pushed a commit that referenced this pull request Jun 7, 2022
RustCrypto is preferred, as it's a pure-Rust implementation, which
simplifies cross compilation for us and prevents us from needing to
pull in all of OpenSSL.

We originally thought that OpenSSL would be required here as RustCrypto
didn't appear to support RSA decryption without padding, but that
turned out to be false.
@LKozlowski LKozlowski deleted the lkozlowski/remove-openssl-with-rdp-without-openssl branch June 8, 2022 09:01
@webvictim webvictim mentioned this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop-access documentation rdp release-notes A reminder label to into the release notes of the Teleport Release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants