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

RFD 136 - Modern Signature Algorithms #28110

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

nklaassen
Copy link
Contributor

@nklaassen nklaassen commented Jun 21, 2023

This is an RFD on modernizing the signature algorithms used in Teleport.

@nklaassen nklaassen force-pushed the rfd/0136-modern-signature-algorithms branch from 8ea04f3 to 759b20f Compare June 28, 2023 23:09
@nklaassen nklaassen marked this pull request as ready for review June 28, 2023 23:10
@github-actions github-actions bot added rfd Request for Discussion size/lg labels Jun 28, 2023
@github-actions github-actions bot requested review from jakule and rudream June 28, 2023 23:11
Copy link
Contributor

@klizhentas klizhentas left a comment

Choose a reason for hiding this comment

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

@nklaassen I would like to understand the following:

  • What would be the new defaults compared to the previous release
  • How many new permutations we are adding with this change
  • Is this really necessary to add so many new configuration options? For example, instead, can we set new config version V7 that will switch to new defaults?

I would like to get Doynsec's opinion on this before we proceed with implementation as well.

@nklaassen
Copy link
Contributor Author

nklaassen commented Jun 29, 2023

@klizhentas

@nklaassen I would like to understand the following:

  • What would be the new defaults compared to the previous release

My apologies, this is not clear in the current version of this doc, I will clarify in the doc and include my idea in this reply.

Edit: I have added a Summary section near the top of the RFD with a better version of what I originally wrote here

  • How many new permutations we are adding with this change

Hmm I'd have to remember my combinatorial math but safe to say A LOT. But users who stick with the recommended algorithms will all have the same setup, they should only deviate from that for a specific need.

  • Is this really necessary to add so many new configuration options? For example, instead, can we set new config version V7 that will switch to new defaults?

We can change all of the recommended values at once in either a new config version or a major release. For most users they will completely ignore this config, never see it, and everything will work fine for them with the new algorithms the next time that they do a CA rotation.

This huge config is really just an "opt-out" mechanism per protocol to lock in a specific algorithm that we won't change from under you. My opinion is that customers with compliance or compatibility requirements that aren't met by our recommended algorithms will really need this. If we just unilaterally change all algorithms in config vN, the only way to opt out will be to stay on config vN-1 which will prevent our customers from upgrading to vN+1 when we eventually release the next feature that requires a config version bump.

I would like to get Doynsec's opinion on this before we proceed with implementation as well.

I will figure out how to get this in front of Doyensec

@nklaassen nklaassen requested review from espadolini, klizhentas and jentfoo and removed request for rudream June 29, 2023 19:02
* `ECDSA_P256_SHA256` has Go BoringCrypto support
* some environments still require RSA

#### JWT CA
Copy link
Contributor

Choose a reason for hiding this comment

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

The JWT, OIDC and SAML keys kind of terrify me a bit, but I guess that changing the algorithm and rekeying them if one encounters problems with some third party app/service is relatively low-effort.

@mdwn do you know if ECDSA support is widespread enough in the SAML ecosystem that we can default to it?

@klizhentas
Copy link
Contributor

klizhentas commented Jul 1, 2023

@nklaassen I would like to reduce the abundance of new options that increases odds of customers to break something or end up with insecure combination.

99% of users should not think and get the most secure up-to-date configuration by default with new version of teleport or upgrade. Users should get only secure configuration options in a batch or not get anything at all.

@jentfoo
Copy link
Contributor

jentfoo commented Jul 5, 2023

@klizhentas Do you have anything in mind for how we should present this configuration?

The current definition does have a lot of configuration options but it also provides users with the greatest flexibility.

I tried to think of alternatives to how we could present this configuration and the best idea I have is a configuration of "Cipher Suites" that you could then select. This could allow us to version our cipher suites, and make the configuration simpler, but does have some disadvantages:

  • If a user has an integration that prevents a specific cert from being updated they either must:
    1. Specify multiple suites (which if the older algorithms are also supported there is a limited security advantage in doing any update)
    2. Be stuck on an older suite for all algorithms
  • Fips suite must also be considered carefully. If we allow combining suites we would want to make sure the fips suite has special rules to prevent incompatible algorithms

I understand the concern about the number of configuration options, but I don't have any ideas of how the configuration could be presented in a simpler way that both allows compatibility to be retained while also maintaining the security benefit of updating your algorithms.

One possible MVP option would be to start with the simpler configuration "suites" idea, but don't allow combining multiple suites. That would force users in the first bullet to only choose option ii, but provide the strongest security gains. Compatibility could be later added as either additional configuration options, or accepting multiple suites if necessary.

@klizhentas
Copy link
Contributor

@jentfoo

One possible MVP option would be to start with the simpler configuration "suites" idea

This sounds good to me. I think of it as a "presets" - by default, we give you the most secure suite possible. Then there is a "fedramp" suite, and then the current, "legacy" one. We may add other suites/presets in the future.

The suite applies to all points where we encrypt, sign, verify, etc at the same time. IT gives a stronger invariant and guarantee that users haven't misconfigured anything.

Copy link
Contributor

@jentfoo jentfoo left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor suggestions to consider

Copy link
Contributor

@klizhentas klizhentas left a comment

Choose a reason for hiding this comment

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

UX looks good to me. Approved pending security review by Doyensec, please review the proposed cipher suites and algorithms and risks associated with each.

* `legacy`
* `balanced-v1`
* `fips-v1`
* `hsm-v1`
Copy link
Contributor

Choose a reason for hiding this comment

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

No 'strict - here is my foot' options? 😅

@nklaassen nklaassen requested review from jakule and espadolini July 21, 2023 16:41
Copy link
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

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

It would be nice to have a table with the signature algorithms we're going to use on each preset.

Copy link

@mbaraniak-doyensec mbaraniak-doyensec left a comment

Choose a reason for hiding this comment

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

Looks good to me

@nklaassen nklaassen force-pushed the rfd/0136-modern-signature-algorithms branch from 46e8788 to 58086ea Compare July 28, 2023 19:44
@nklaassen nklaassen enabled auto-merge July 28, 2023 19:51
@nklaassen nklaassen added this pull request to the merge queue Jul 28, 2023
Merged via the queue into master with commit a6dd9c3 Jul 28, 2023
@nklaassen nklaassen deleted the rfd/0136-modern-signature-algorithms branch July 28, 2023 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfd Request for Discussion size/lg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants