feat!: Make auto-enrollment enabled by default

[codingllama]

Add a new, non-boolean auto_enroll_mode setting to cluster_auth_preference and change the feature behavior to enabled by default.

The old auto_enroll field in cluster_auth_preference is now completely ignored*. Since we can't distinguish "false" from "not set", any values are effectively taken as "true". Disabling auto-enrollment is only possible by setting auto_enroll_mode: disabled.

File-based configurations, like teleport.yaml, have no UX changes (the field was already a string there). Note that the empty semantic does change from "disabled" to "enabled".

cluster_auth_preference:

version: v2
kind: cluster_auth_preference
metadata:
  name: cluster-auth-preference
  # (...)
spec:
  device_trust:
    auto_enroll: true         # "old" field, ignored
    auto_enroll_mode: enabled # "" or "disabled" also allowed.
  # (...)

teleport.yaml (no field name changes):

auth_service:
  authentication:
    device_trust:
      mode: required
      auto_enroll: true # still valid, as are "", "enabled", "disabled"
                        # and various boolean-like values.

* Once the e/ counterpart to this PR lands.

[codingllama]

Note to reviewers: think of this PR less as a done deal, and more like a proposal. None of this is set in stone.

FYI @zmb3 @klizhentas @sshahcodes, this is a way in which we could change auto-enrollment to enabled by default. Let me know your thoughts.

[codingllama]

See https://github.com/gravitational/teleport.e/pull/2043 for Enterprise changes.

[codingllama]

Friendly ping @zmb3 @camscale @rosstimothy ?

[zmb3]

Sorry Llama - busy day. I'll put this on the list for tomorrow.

[codingllama]

@zmb3 no worries, it's just the "automated" 1-workday ping so y'all don't forget about me.

[codingllama]

(Rebased and fixed protogen conflicts.)

[camscale]

LGTM. It's a shame there's no way to know whether the user explicitly disabled auto enrollment as switching an explicitly disabled setting to enabled would be surprising. I wonder if it might be better to phase this new field in and set AutoEnrollMode to the value of AutoEnroll and in a subsequent version switch the default to enabled. Then we can say any new AuthPreference created after version X will default to enabled, but if created before that it will be disabled and stay that way. That still has problems for users that skip the interim version though. The alternative would be AuthPreferenceV3 which has the new default.

Are there any nasty security implications switching on auto enrollment for a user that has explicitly disabled it?

[camscale]

This comment doesn't seem to be completely true. If AutoEnroll == true and AutoEnrollMode == disabled then AuthPreferenceV2.CheckAndSetDefaults() will return an error.

[codingllama]

I think that's alright? In most cases it is ignored and the error explains itself.

[codingllama]

Adding to the reply, I don't want to explain too many caveats. The error could change or be dropped, but the main reason for the comment remains: do not use or rely on the old flag.

[codingllama]

@camscale

LGTM. It's a shame there's no way to know whether the user explicitly disabled auto enrollment as switching an explicitly disabled setting to enabled would be surprising.

That's what I get from using a boolean where I shouldn't have. :/

I wonder if it might be better to phase this new field in and set AutoEnrollMode to the value of AutoEnroll and in a subsequent version switch the default to enabled. Then we can say any new AuthPreference created after version X will default to enabled, but if created before that it will be disabled and stay that way. That still has problems for users that skip the interim version though. The alternative would be AuthPreferenceV3 which has the new default.

The feature is seeing little use atm, so a quick switch might be for the best.

We could do a more careful, paced approach, or do a "v3" switch like you said, but I expect it would catch people by surprise anyway. Another problem with a phased, "v3" approach is that it's unlikely to get us to where we want, as in auto-enroll being the default (lots of "older" configs will stay as they are).

Are there any nasty security implications switching on auto enrollment for a user that has explicitly disabled it?

If you keep your device inventory in Teleport this means that users can suddenly enroll themselves. Keeping the inventory requires effort, it's not something that just happens, and if you do that you likely want the means to enroll. Auto-enroll is the more practical solution, so it is largely desirable (most customers want something like it). That said, the security guarantees do change a bit.

[codingllama]

@klizhentas, if you have a moment it would be good to have a product perspective on this change.

[rosstimothy]

What kind of compatibility issues might arise from this switch? What happens if you rollback from this change?

[codingllama]

@rosstimothy

What kind of compatibility issues might arise from this switch? What happens if you rollback from this change?

In fileconf, if you are using a boolean, semantics remain the same on a rollback. If you are using one of the "new" values ("enabled" or "disabled"), then it'll break. I'm not sure there's much we can do there.

For cluster_auth_preference, the new field will be unknown so you'll be back to disabled auto-enroll.

[codingllama]

@camscale

If you keep your device inventory in Teleport this means that users can suddenly enroll themselves

A slight correction of myself: enrolling devices requires the device/enroll verb, so it's not something that "just happens" either - users have to be assigned to a role that lets them do it.

[rosstimothy]

@rosstimothy

What kind of compatibility issues might arise from this switch? What happens if you rollback from this change?

In fileconf, if you are using a boolean, semantics remain the same on a rollback. If you are using one of the "new" values ("enabled" or "disabled"), then it'll break. I'm not sure there's much we can do there.

For cluster_auth_preference, the new field will be unknown so you'll be back to disabled auto-enroll.

What would Cloud be using? We want to make sure we don't brick them if they have to downgrade.

[codingllama]

What would Cloud be using? We want to make sure we don't brick them if they have to downgrade.

cluster_auth_preference, I expect? Why would they "brick"?

[rosstimothy]

What would Cloud be using? We want to make sure we don't brick them if they have to downgrade.

cluster_auth_preference, I expect? Why would they "brick"?

If they use the file conf option and set the field to enabled wouldn't that brick if they had to downgrade?

[codingllama]

How is that different than any other new config value?

Do you have an alternative in mind?

[rosstimothy]

How is that different than any other new config value?

Do you have an alternative in mind?

I'm not saying that it is. We just need to make them aware of the consequences so they aren't caught off guard.

[zmb3]

I haven't looked at the code, but I am concerned that auto_enroll: true is the canonical way to enable auto enrollment in teleport.yaml, but has no affect in the cluster auth preference.

CAP and teleport.yaml usually use the same fields, so breaking from that precedent here is likely to confuse people, don't you think?

[codingllama]

I agree, I don't like that either, but I'm not seeing a whole lot of alternatives.

We can default auto-enroll to "true" if there's no device trust section, but as soon as you create one the ambiguity starts. I'm open for suggestions.

[klizhentas]

tbh, I think it's not ideal for a couple of reasons:

• We introduce additional knob, but don't deprecate the old one
• The new parameter is similar to the old one, still used in RBAC, but deprecated and ignored in the cluster auth preference.

As an alternative, I suggest we do nothing for now. In the future, can we simply set auto_enroll: true in cluster auth preference v3?

[codingllama]

Fair enough, closing for now.