extended dynamic access

[fspmarshall]

See the Updated PR Description

Old PR description (hidden)

This is a WIP implementation of part of various improvements to the dynamic access (workflow) API, based primarily off of [rfd/0003](https://github.com//pull/4305). Field/option naming is subject to change.

Overview

Pattern-based allow/deny

Allow/deny blocks for role requests now support regex and blob patterns:

kind: role
metadata:
  name: contractor
spec:
  allow:
    request:
      roles: ['^customer-.*$']
  deny:
    request:
      roles: ['*-account-admin']

Request and resolution reasons

Requests can now be generated with a RequestReason field:

$ tsh login --request-roles=admin --request-reason='I desire power'

Seeking request approval... (id: 99abaf3e-d52e-41ce-8ae3-fe6a9868a561)

When a request is "resolved" (approved/denied) a reason can be given for that too:

$ tctl request deny --reason='I do not share power' 99abaf3e-d52e-41ce-8ae3-fe6a9868a561

note: the default formatter has not been updated yet, so use tctl request ls --format=json to view the reason fields associated with requests.

Resolution attributes

Arbitrary attributes can now be associated with a resolution for automated bookkeeping:

$ tctl request deny --attrs=foo=bar,spam=eggs a2262fd5-30f1-43f1-ac8c-359bb17cae3a

Automatic role selection and overrides

An access request can now have its roles auto-populated by the auth server:

$ tsh login --request-roles=*

This will cause the access request to automatically include all roles that are permissible for the user to assume. This is also going to be the default behavior used by the web UI's "waiting room" (see #4384).

If the approving party does not wish the user to assume all of the specified roles, the role list can now be overridden at the time of resolution like so:

$ tctl request approve --roles=foo,bar a2262fd5-30f1-43f1-ac8c-359bb17cae3a

note: Role list overrides don't circumvent the requesting user's allow/deny directives. The user must be allowed to assume the specified roles in order for the override to succeed.

New role options

Two new boolean role options have also been added:

kind: role
metadata:
  name: contractor
spec:
  options:
      # hint to the web UI indicating that this role should always request
      # escalated privilege.
      auto_request_access: true

      # require that all requests for escalated privilege include a reason.
      require_request_reason: true

[klizhentas]

@fspmarshall the reason I've proposed to have

request_access: optional|always|note is because in case of always users are not seeing the note, the request is created automatically. note requires a note all the time. optional is a default value will be allowed in open source (our current behavior). note and always will be only available in the enterprise

In your design for roles use slightly different config. But in your example above - this combination where both auto_request_acces: true and require_request_reason: true are conflicting, because we can't request a note if auto_request_access is true - we should automatically create access request without asking. We can discuss these details later in the week.

      # hint to the web UI indicating that this role should always request
      # escalated privilege.
      auto_request_access: true

      # require that all requests for escalated privilege include a reason.
      require_request_reason: true

@fspmarshall ping on my question above

[fspmarshall]

this combination where both auto_request_acces: true and require_request_reason: true are conflicting, because we can't request a note if auto_request_access is true - we should automatically create access request without asking.

That is not the intent. The intent is that if both are true, the dialogue for adding a note atomatically appears (i.e. if both are true it is equivalent to request_access: note). If only require_request_reason is true, we keep the current behavior (access requests are generated only if the user explicitly triggers them), but we require that if a request is triggered, it includes a reason/note. This is why I separated them. I wanted the ability to represent all four possible states.

That being said, this was just an experiment. Its possible that requiring a note when not auto-requesting is too niche, we can drop it in favor of the single field.

[klizhentas]

@fspmarshall I got it. I prefer encapsulating this logic in one option, request_access and reducing the amount of behaviors and combinations to 3:

# allow user to request access if `can_access` is setup for the role  (default value for all roles)
request_access: 'optional'
# create access request after asking for note if `can_access` is setup for the role
request_access: 'reason' 
# always create access request after login without asking for note if `can_access` is setup for the role
request_access: 'always'

We can always add more options and combinations in the future.

[klizhentas]

@fspmarshall thoughts for tomorrow conversation:

Add claims to access_request

Adding traits to access_request allows plugins to map users to roles dynamically.

• In OSS version, claims contains a list of Github teams in github connector

access_request:
   spec:
      claims: ['gravitational/devc']

In enterprise, contains OIDC claims or SAML attribute statements:

access_request:
   spec:
      claims: {'groups': ['dictators', 'populists']}

Add mapped_roles to access_request

This field contains the roles user is already assigned by Teleport after login.

access_request:
   spec:
       mapped_roles: ['dictator', 'populist']

Imagine a user already is assigned to roles dictator and populist. The company then is facing a reulatory requirement to request a ticket number for every user. The company admin adds just one role

role:
  name: requestor
  spec:
     options:
         request_access:  'note'
     allow:
        request:
           roles: ['.*']

and modifies the connector mapping:

connector:
   spec: 
   claims_to_roles:
     - claim: "group"
       value: ".*"
       roles: [ ".*", "requestor"]

Because of the 'requestorevery user will be forced to login with a note, and this will generate the followingaccess_request`:

access_request:
   spec:
     claims: {'groups': 'dictator'}
     mapped_roles: ['dictator']
     # in this case we don't need to populate roles with any matching roles
     # what makes the system more resilient
     roles: []

The plugin can take a look at claims and see if likes the roles user is already mapped and then simply:

request.Roles = request.MappedRoles
request.State = Approved

In this case we won't need to populate roles from can_request automatically.

[fspmarshall]

@klizhentas Still mulling this over, but my immediate impression it that the above is going to require some tricky plumbing.
Especially because up to this point access requests have had no special relationship with the login flow. That being said, I'm fairly certain this would achieve almost the same effect much more simply with something like this:

kind: oidc
metadata:
  name: "connector"
spec:
  claims_to_roles:
     - claim: "group"
       value: "employee"
       roles: [ "employee" ]
  # ...
---
kind: role
metadata:
  name: employee
spec:
  allow:
    request:
      roles: ['{{external.groups}}']
  options:
    request_access: 'note'

The plugin would see a request for all roles matching the groups held by the user (minus employee since the auth server filters out roles already held). The plugin can then choose to grant all or a subset.

We could also provide a simple mechanism for propagating side-band information directly to the plugin via the existing variable interpolation system (again, without much effort, and with no special cases in the login system):

kind: role
metadata:
  name: employee
spec:
  allow:
    request:
      roles: ['{{external.groups}}']
      # pass some extra metadata to the plugin; nothing special, just a mapping that obeys
      # our existing variable interpolation system.
      meta:
        tickets: '{{external.tickets}}'
  options:
    request_access: 'note'

[klizhentas]

@fspmarshall your suggestion is better, let's use it!

[klizhentas]

kind: oidc
metadata:
  name: "connector"
spec:
  claims_to_roles:
     - claim: "group"
       value: ".*"
       roles: [ "catchall" ]

kind: role
metadata:
  name: catchall
spec:
  allow:
    request:
      roles: ['{{external.groups}}']
    # pass some extra metadata to the plugin; nothing special, just a mapping that obeys
    claims_to_roles:
      - claim: "group"
         value: ".*"
         roles: [ "admins" ]
   options:
    request_access: 'note'
    request_prompt: |
       Hey, Enter the Ticker number from https://ibm.com/tickets
       ....

Usual role

kind: role
metadata:
  name: admins
spec:
  allow:
    logins: ['root']

access_request:
   spec:
     reason: 'ticket #1234`
     roles: ['admins', 'devops']

Plugin processes the request:

access_request:
   spec:
     note: 'ticket #1234`
     roles: ['admins', 'devops']
     state: approved

[klizhentas]

@fspmarshall @benarent @kimlisa

After giving our last idea some thought, pushing claims_to_roles to role spec seems wrong.

It duplicates the logic of the connector and introduces ambiguity. What happens if there are two roles with claims_to_roles mapping? We would have to figure out a way to prioritize the role set. Same for conflicting prompts, etc.

What if we do a reverse, move request parts to SAML and OIDC connectors.

In some cases, there is no need for claims_to_roles mapping, could be done by the plugin:

connector:
   spec:
      request:
          mode: always
          prompt: |
Enter the ticket number.

In other cases, one change will start requesting reason for everyone after doing the mapping if the role allows it:

connector:
   spec:
      claims_to_roles:
      - claim: group
         value: .*
         roles: ['requestor']
      request:
          mode: reason
          prompt: |
Enter the ticket number or free form reason

[klizhentas]

@fspmarshall how about this:

1. Let's forget the connector changes, our connector looks like this:

connector:
   spec:
      claims_to_roles:
      - claim: groups
         value: '.*'
         roles: ['requestor']

2. Going back to your original variables proposal

role:
   name: requestor
    spec:
       options:
           request_access: 'note'
       allow:
          request:
             # used claims to roles generator
             # instead of static role list
             claims_to_roles:
                - claim: 'groups'
                   value: '.*'
                   roles: ['dictator', 'populist']

Generates access request:

access_request:
     spec:
        roles: ['dictator', 'populist']

[fspmarshall]

Update

Role conditions have been updated to allow mapping claims (traits) to construct the role list dynamically. This allows the access request system to determine which roles a user should be allowed to request based on information passed in from the identity provider. In order to simplify the upgrade process, we've kept the mapping syntax identical to that found in the oidc connector resource:

kind: role
metadata:
  name: employee
spec:
  allow:
    request:
      claims_to_roles: # same as it would appear in an oidc connector
        - claim: groups
          value: dev
          roles: ["dev-prod", "dev-staging"]

Because the request.claims_to_roles mapping is used to construct the actual request.roles value, which is then itself used to determine the allowable roles, it supports the interpolation syntax just like a normal request.roles list would:

kind: role
metadata:
  name: employee
spec:
  allow:
    request:
      claims_to_roles:
        # allow admins to request all roles which do not end in '-restricted'
        - claim: groups
          value: admin
          roles: ['{{regexp.not_match(".*-restricted")}}']

note: the above roles are all minimal examples and don't represent all options needed to create a secure entrypoint role. See the restricted role UX improvements notes in the 'further work' section below.

---

The auto_request_access and require_request_reason role options have been collapsed into a single request_access field as discuessed here:

kind: role
metadata:
  name: contractor
spec:
  options:
    request_access: reason

---

tsh has been updated to obey the request_access field. If --request-roles is not specified, it will generate a wildcard request, which will cause the plugin to see a request for all roles that the user is permitted to request. The plugin will see the role list with all mappings and match syntax already applied, so no special knowledge of mappings or existing roles are required.

---

Request resolution attributes have been changed from map[string]string to map[string][]string to better reflect a typical claims-esque datastructure.

---

Audit events now support reason and attrs fields (only present if non-empty).

Request creation event:

{
  "event": "access_request.create",
  "user": "alice",
  "roles": [ "dictator" ],
  "id": "5c5d1924-97fc-4ef6-aef1-fcf32c5c8152",
  "state": "PENDING",
  "reason": "I desire power",
  ...
}

Request approval event:

{
  "event": "access_request.update",
  "updated_by": "bob",
  "id": "5c5d1924-97fc-4ef6-aef1-fcf32c5c8152",
  "state": "APPROVED",
  "reason": "You seem trustworthy",
  "attrs": {
    "hello": [ "world" ]
  },
  ...
}

---

Deferred work

• We're going to be skipping direct trait interpolation within the statically defined roles list for now. The claims_to_roles mapping is much more powerful and does basically the same thing.

---

Further work needed

• Gather feedback on current iteration.
• Test coverage of all new features, most importantly the updated role mapping & matching.
• Improve UX around creating a minimal restricted role. Including, but probably not limited to, removing the restriction that all users must have a static login, and providing a clear and easy mechanism to define a role with no node labels without accidentally giving you users wildcard allow (good catch @kimlisa).
• Probably other stuff that I'm forgetting right now...

[benarent]

@fspmarshall I did a quick review and it looks good. As I think though the docs / setting this up. I thinking about these things.

1. When testing / setting up, how do I know if the user I want has the correct traits -> roles? Maybe we could start with a more verbose debug log for this. ( This is really out of scope for this, but if we can improve it'll be great )
2. Do you have to setup the OIDC/SAML connecter to obtain the groups used in these roles? Or will the Role be enough.
3. What happens if the OIDC/SAML returns nothing? Do we just fail the login if the user has no other limited users / groups.

[fspmarshall]

When testing / setting up, how do I know if the user I want has the correct traits -> roles?

Good question. The best way to do this is to test with the real configuration in a staging env. However, if you don't have the ability to do this, I think I have an idea for a fairly simple way to make testing easier...

Just pushed an experimental commit that adds a --dry-run flag for tctl request create which will run the basic validation and variable expansion without actually creating the described request. This now works:

$ cat > rscs.yaml <<EOF
kind: user
metadata:
  name: test-user
spec:
  roles: ['employee']
  traits:
    # put the claims/traits that you'd like to test against here
    groups: ['admins']
version: v2
---
kind: role
metadata:
  name: employee
spec:
  allow:
    request:
      claims_to_roles:
        - claim: groups
          value: admins
          roles: ['*-admin']
version: v3
EOF

$ tctl create rscs.yaml 
# ...

$ tctl request create --dry-run test-user | jq -r '.[].spec.roles[]'
staging-admin
prod-admin
# ...

Thoughts?

---

2. Do you have to setup the OIDC/SAML connecter to obtain the groups used in these roles? Or will the Role be enough.

Same process as passing data to teleport's variable interpolation system (e.g. {{external.unix_logins}}). I think most identity providers have some set of claims that they send by default, but generally people will have custom claims that they have created with their identity provider, which must be requested via their scopes. E.g.

kind: oidc
metadata:
  name: my-odic
spec:
  scope: ['some-custom-scope', 'some-other-custom-scope']

---

3. What happens if the OIDC/SAML returns nothing? Do we just fail the login if the user has no other limited users / groups.

Login and access request are separate. If you are generating an access request, you are already logged in. That being said, if the role is configured to automatically generate an access request after login (e.g. request_access: always), then the client-side code will treat failing to get an approval as being equivalent to failing to login and won't save the intermediate certificates. This isn't a security thing though, its just a user-experience thing. The assumption is that if the role says that access requests should always be generated, then the role probably doesn't grant enough privileges to be very useful pre-escalation.

[kimlisa]

Couple of things:

• still need request_prompt option
• could we also sort request ls by create date, with most recent on top

[benarent]

Just pushed an experimental commit that adds a --dry-run flag for tctl request create which will run the basic validation and variable expansion without actually creating the described request. This now works:

This is super helpful and is another step to helping with #3845, and will make debugging this much easier. I take that I can use this with any user in the system ( if they have been created / logged in via SSO )

[benarent]

Thanks for the comments @fspmarshall once we are the 'beta' stage of this PR, I'll setup a staging site so we can fully test with an IDP

[fspmarshall]

Updated PR Description

This PR includes various changes designed to significantly increase the power of the dynamic access (workflow) API. Most of these changes revolve around supporting two key features:

• Supporting users that start in an essentially unprivileged state and must always go through the dynamic access API in order to gain meaningful privilege.

• Leveraging the claims (traits) provided by external identity providers both when determining which roles a user is allowed to request, and if a specific request should be approved/denied.

RBAC

Roles now support a number of new or updated configuration fields:

kind: role
metadata:
  name: employee
spec:
  allow:
    request:
      # the `roles` list can now be a mixture of literals and matchers
      roles: ['common', 'dev-*']
      # the `claims_to_roles` mapping works the same as it does in
      # the oidc connector, with the added benefit that the mapped to roles
      # can also be matchers.  the below mapping says that users with
      # the claims `groups: admins` can request any role in the system.
      claims_to_roles:
        - claim: groups
          value: admins
          roles: ['*']
      # teleport can attach annotations to pending access requests. these
      # annotations may be literals, or be variable interpolation expressions,
      # effectively creating a means for propagating selected claims from an
      # external identity provider to the plugin system.
      annotations:
        foo: ['bar']
        groups: ['{{external.groups}}']
  options:
    # the `request_access` field can be set to 'always' or 'reason' to tell
    # tsh of the web UI to always create an access request on login.  If it is
    # set to 'reason', the user will be required to indicate *why* they are
    # generating the access request.
    request_access: reason
    # the `request_prompt` field can be used to tell the user what should
    # be supplied in the request reason field.
    request_prompt: Please provide your ticket ID
version: v3

Notice that the above role does not specify any logins. This is OK. If a users's roles specify no logins, teleport will now generate the user's initial SSH certificates with an invalid dummy login of the form -teleport-nologin-<uuid> (e.g. -teleport-nologin-1e02dbfd-8f6e-47a0-a66c-93747b010f88). This is obviously a bit hacky, but unfortunately some ssh implementations consider a certificate with no principals to be valid for all principals, so we take no chances.

API

A number of new parameters are now available that allow the plugin or administrator to grant greater insight into approvals/denials:

$ tctl request deny --reason='you seem sus' --annotations=method=cli,unix-user=${USER} 28a3fb86-0230-439d-ad88-11cfcb213193

Because automatically generated requests always include all roles that the user is allowed to request, approvers can now specify a smaller subset of the requested roles that should actually be applied, allowing for subselection in cases where full escalation is not a desirable default:

$ tctl request approve --roles=role-1,role-3 --reason='thats cool, but no role-2 right now' 28a3fb86-0230-439d-ad88-11cfcb213193

Events

The access_request.create and access_request.update events have been extended to include reason and annotations fields.

Request creation event:

{
  "event": "access_request.create",
  "user": "alice",
  "roles": ["dev-staging", "dev-prod"],
  "id": "5c5d1924-97fc-4ef6-aef1-fcf32c5c8152",
  "state": "PENDING",
  "reason": "ticket #12345",
  "annotations": {
    "groups": ["developers","book-club"],
    ...
  },
  ...
}

Request approval event:

{
  "event": "access_request.update",
  "updated_by": "bob",
  "id": "5c5d1924-97fc-4ef6-aef1-fcf32c5c8152",
  "state": "APPROVED",
  "roles": ["dev-staging"],
  "reason": "non-emergency",
  "annotations": {
    "ticket": ["12345"],
    ...
  },
  ...
}

[russjones]

Won't this make this check always pass? I'd either look into making sure services.RoleSet has a role with a (dummy) login or maybe passing a flag to this function that control this behavior.

[fspmarshall]

No, the logins list there has been filtered down. This check is used to distinguish between the case where the user does have logins, but none match, and the case where the user actually has no logins. We're only generating the dummy login for the case that wasn't previously allowed (actually having no logins), which keeps us backwards-compatible.

[russjones]

Protobufs support a enum type.

https://github.com/gravitational/teleport/blob/master/lib/services/types.proto#L25-L32

[fspmarshall]

Those serialize to integers by default, and this field needs to be a string when in json or yaml format since users are expected to manually modify the field. I could write a custom marshaler for it, but IMO the benefit is negligible. Especially since go doesn't have enums, so we end up with a bunch of constants under the hood anyhow.

[russjones]

Okay, makes sense.

[klizhentas]

style nit (optional), not sure how much append simplifies the logic, if you have collectMatchers that has no side effects you can always do:

matchers = append(matchers, collectMatchers(allow, traits)...)

[klizhentas]

Looks solid. I haven't found logical flaws. I'd encourage to add calls to actions to make API callers and developers life easier. Otherwise, 👍

[klizhentas]

Good thinking

[russjones]

Looks good to me pending applying remaining comments.

[russjones]

I don't think you need this label here, continue should continue to the next value in a inner loop.

[fspmarshall]

I try to always use loop labels if using continue within nested loops. IMO its more readable and less prone subtle bugs due to later changes or bad merges.

[russjones]

Okay, makes sense.