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

[Fleet] Add permission specs to policy #94058

Closed
ruflin opened this issue Mar 9, 2021 · 19 comments · Fixed by #94591
Closed

[Fleet] Add permission specs to policy #94058

ruflin opened this issue Mar 9, 2021 · 19 comments · Fixed by #94591
Assignees
Labels
Team:Fleet Team label for Observability Data Collection Fleet team

Comments

@ruflin
Copy link
Member

ruflin commented Mar 9, 2021

To reduce the permissions each Elastic Agent requires, the permission on each API Key that is handed out to the Elastic Agent must be reduced. In elastic/fleet-server#101 a proposal is discussed which requires that each policy contains a definition on what permissions are need to run this policy. Based on these, the fleet-server creates API keys.

As the policy is created in Kibana, this permissions block would have to be added to the policy by Fleet. The permissions should be created based on the integrations in a policy. To know which permissions are required, each package should contain a definition on what it requires. In addition, special flags could be set on a policy to allow dynamic fields or wildcards to increase permissions.

In a first phase, the implementation could be to just add our default permissions to each policy. With this, we put the foundation in place to further iterate on it and reduce the permissions over time. Also it means fleet-server can build in all the components and can stop worrying about the permissions as it becomes a Fleet issue.

@ruflin ruflin added the Team:Fleet Team label for Observability Data Collection Fleet team label Mar 9, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@ph
Copy link
Contributor

ph commented Mar 9, 2021

@ruflin I've assigned @afgomez to this issue, just to clarify.

n a first phase, the implementation could be to just add our default permissions to each policy. With this, we put the foundation in place to further iterate on it and reduce the permissions over time. Also it means fleet-server can build in all the components and can stop worrying about the permissions as it becomes a Fleet issue.

Do you mean current permission for "metrics-*, logs-*-* but returned as part of the policy? elastic/fleet-server#101

@ruflin
Copy link
Member Author

ruflin commented Mar 10, 2021

Yes

@afgomez
Copy link
Contributor

afgomez commented Mar 15, 2021

I wrote a little something to help me clarify my thoughts. Let me know if I'm missing something

We want Fleet server to generate API keys for the agents based on whatever permissions they need.

The current situation

Fleet server generates an API key with coarse permissions. All agents get the same permissions.

What do we want?

Policies should specify the permissions they need based on their integrations. For example, if a policy uses Elastic APM, it should have permissions to traces-apm* instead of all the traces-* indices.

Fleet server will then read these permissions and use them when it generates the API key for the agent.

Approach

1. Move hardcoded permissions to the policy itself (#94591)

The first step will be to hardcode the current coarse permissions when generating the policy. The permissions will be added on a new key called outputPermissions, with the following type signature.

interface FullPolicy {
  // ...
  outputPermissions?: {
    [key: string]: Array<{ names: string[]; privileges: string[]; }>
  } 
}

Since there is an option to have several outputs, there should be an option to have several output permissions.

We do it as a separate key to not fiddle with the current outputService (which handles the outputs). The permissions will come from the integrations, which AFAIK, the outputs have no concept at all.

The fleet server will use the outputPermissions to generate the granular API keys for the agents that use those policies.

2. Specify permissions in each integration.

The integrations will then specify which privileges they need for which indices. The outputPermissions will be built based on an aggregate of all permissions of all integrations. How to do this will be defined in due time.

At this step, if an integration doesn't have a permissions key we will inject the default permissions and warn the user. This is to help migration of all integrations.

A possible sane default could be to automatically provide the create_doc privilege to all indexes used by the integration, although this might break things if they rely on other privileges.

@ruflin
Copy link
Member Author

ruflin commented Mar 16, 2021

  1. Good point around multiple output support. There will be indeed at one stage support for multiple outputs. Good to have the fundamentals in place to expand to this in the future.
  2. I expect that the permissions will be specified on the data_stream and not necessarily the package level. As you mentioned, we should come up with a sane default if nothing is specified. We know in advance all the indices that will be needed because of the dataset names. But I would delay this discussion until we have the basic implementation in.

Could you share an example in json or yaml of the permission block? This is to make sure fleet-server will know exactly what to implement.

@afgomez
Copy link
Contributor

afgomez commented Mar 16, 2021

Could you share an example in json or yaml of the permission block?

Sure!

# ...
revision: 1
outputs:
  default:
    type: elasticsearch
    hosts:
      - 'http://localhost:9200'
outputPermissions:
  default:
    - names:
        - logs-*
        - metrics-*
        - traces-*
        - .logs-endpoint.diagnostic.collection-*
      privileges:
        - auto_configure
        - create_doc
# ...

@ruflin
Copy link
Member Author

ruflin commented Mar 16, 2021

Looking at https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-create-api-key.html#security-api-create-api-key-example I wonder if we will ever need the cluster part. And who takes care of the "roles"? Will fleet-server have to "generate" roles or will they also be passed down as part of the policy with some names?

@nchaulet
Copy link
Member

For API keys you give a role descriptor there is no need to create any roles, I am wondering if we should have a structure that match more that concept so we can easily have different privileges for different index

outputPermissions:
  default:
     role_descriptors: 
        role_1: 
           names: 
                - metrics-*
                ...
        role_2: 
           names: 
                - logs-endpoint.diagnostic.collection-*

@ruflin
Copy link
Member Author

ruflin commented Mar 16, 2021

Should have been more specific, I meant the description. What you put in is now almost the full ES part. Going further, I wonder if we should go so far and even add the top level part around expiration so fleet could also add an expiration on the policy. The advantage of taking the full block would be that we are prepared for any non breaking addition from the Elasticsearch side in case we ever need it. Main downside is that it is pretty verbose and the structure as potentially info inside we never need..

@afgomez
Copy link
Contributor

afgomez commented Mar 17, 2021

I am wondering if we should have a structure that match more that concept so we can easily have different privileges for different index.

Maybe it's not obvious because of the yaml syntax, but we can do that with the proposed structure. Each key inside outputPermissions contains an array.

outputPermissions:
  default:
    - names:
        - logs-*
      privileges:
        - auto_configure
        - create_doc
    - names: 
        - metrics-*
      privileges:
        - create_doc
        - read
        - ...  

The type signature is as follows

interface FullPolicy {
  // ...
  outputPermissions?: {
    [key: string]: Array<{ names: string[]; privileges: string[]; }>
  } 
}

I wonder if we should go so far and even add the top level part around expiration so fleet could also add an expiration on the policy

Is there a use case where different policies could benefit from different expiration times? Somehow I feel it's more of an implementation detail on the fleet side that something that should be in the policy.

If we want to make sure to leave the door open we can change the type to something like this:

interface FullPolicy {
  // ...
  outputPermissions?: {
    [key: string]: {
       indices: Array<{ names: string[]; privileges: string[]; }>
       // add properties as needed in the future
    }
  }
}

But even then, it is possible to leave the legacy version now, and if needed, express a new structure with types.

interface PolicyPermission {
  names: string[];
  privileges: string[];
}

interface FullPolicy {
  // ...
  outputPermissions?: {
    [key: string]: PolicyPermission[] | {
       indices: PolicyPermission[]
    }
  }
}

I personally prefer the very first option because YAGNI, but I'm open to the second.

@afgomez
Copy link
Contributor

afgomez commented Mar 17, 2021

And who takes care of the "roles"? Will fleet-server have to "generate" roles or will they also be passed down as part of the policy with some names?

I would generate it in the Fleet server, something like ${policy.id}:${outputKey}.

Maybe I'm missing something but I feel in this case the role is more something that Elasticsearch needs more than data about the policy itself. I think it should be left out of the policy.

@ruflin
Copy link
Member Author

ruflin commented Mar 17, 2021

@afgomez I agree we can leave out the expiration for now as we don't have a use case for it yet.

I've been thinking a bit more about the roles. Each integration policy inside the policy will have its own permissions. A single integration policy can also have two permission entries as not all indices necessarily have the same permissions (see endpoint example below). Elasticsearch will require a roles descriptor as far as I can see so we need to put something in for the role. We could just generate a random key. But as we know the permissions come from a specific integration policy, woludn't it be nice for debugging purpose to exactly see which permissions are for which integration? This lead me to the following example. It would mean, the role descriptions would have to be created by Fleet.

outputPermissions:
  # What output to pick
  default:
    # Leaving out the role-descriptor part, it is added by fleet-server including the name
    
    # The name of each role is the integration policy id
    nginx-logs-1: 
       names: 
            - logs-nginx.access-*
            - logs-nginx.error-*
       privileges: ["append"]
    nginx-metrics-1: 
       names: 
            - metrics-nginx.substatus-*
       privileges: ["append"]
    endpoint-policy1-part1: 
      names: 
            - .logs-endpoint.diagnostic.collection-*
      privileges: ["read"]
    endpoint-policy1-part2: 
      names: 
            - metrics-endpoint-*
      privileges: ["append"]

@afgomez
Copy link
Contributor

afgomez commented Mar 17, 2021

[...] But as we know the permissions come from a specific integration policy, wouldn't it be nice for debugging purpose to exactly see which permissions are for which integration?

Ok I see that point and I agree.

As an intermediate step for #94591 I can add a role called fallback with the broad permissions, and moving forward add the right roles for each integration.

@ruflin @nchaulet makes sense?

@ruflin
Copy link
Member Author

ruflin commented Mar 17, 2021

++, fallback sounds great.

@ruflin
Copy link
Member Author

ruflin commented Mar 17, 2021

@aleksmaus Can you take a quick look at the above if this would also work well for fleet-server?

@ph
Copy link
Contributor

ph commented Mar 18, 2021

As a related next steps for more "specific integration" permission we have this #64634 issue, I've pull it back into the current iteration since we have the initial version in and we can iteration this is super @afgomez.

@aleksmaus
Copy link
Member

nit: a bit of inconsistency here.
should we change outputPermissions to output_permissions?

Looks like most of the fields use _ convention:
Screen Shot 2021-03-23 at 3 10 44 PM

@afgomez
Copy link
Contributor

afgomez commented Mar 23, 2021

@aleksmaus ah, good point. I'm so used to the camelCase convention in JS that I didn't realise it's different here.

I'll update the PR 👍

@ruflin
Copy link
Member Author

ruflin commented Mar 24, 2021

Small detail I missed in our previous schema discussions:

The following is what is currently put into the policy:

    fallback:
      - names:
          - logs-*
          - metrics-*
          - traces-*
          - .logs-endpoint.diagnostic.collection-*
        privileges:
          - auto_configure
          - create_doc
          

This is what would be compatible with the ES API.

    fallback:
      index:
        - names:
            - logs-*
            - metrics-*
            - traces-*
            - .logs-endpoint.diagnostic.collection-*
          privileges:
            - auto_configure
            - create_doc

The difference is the index part. I understand we don't really need it but at the same time it means fleet-server needs to modify the content and add this entry to the yaml instead of just taking the full yaml block, add some top level fields and ship it to Elasticsearch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Fleet Team label for Observability Data Collection Fleet team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants