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

Update security docs #1511

Merged
merged 2 commits into from
Apr 29, 2021
Merged

Update security docs #1511

merged 2 commits into from
Apr 29, 2021

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Apr 15, 2021

No description provided.

@lkysow lkysow requested a review from a team as a code owner April 15, 2021 21:20
Comment on lines +60 to +61
1. Bake providers into the Atlantis image or host and deny egress in production.
1. Implement the provider registry protocol internally and deny public egress, that way you control who has write access to the registry.
Copy link
Member Author

Choose a reason for hiding this comment

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

@nishkrishnan please help with this section

Copy link
Contributor

Choose a reason for hiding this comment

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

It's worth noting that external data source would likely not be available for use if you don't add it to your internal provider registry. This is an assumption though.

@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

Merging #1511 (e90d011) into master (25103d7) will decrease coverage by 0.12%.
The diff coverage is n/a.

❗ Current head e90d011 differs from pull request most recent head cc0bb8f. Consider uploading reports for the commit cc0bb8f to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1511      +/-   ##
==========================================
- Coverage   70.21%   70.08%   -0.13%     
==========================================
  Files          94       94              
  Lines        6530     6485      -45     
==========================================
- Hits         4585     4545      -40     
+ Misses       1555     1554       -1     
+ Partials      390      386       -4     
Impacted Files Coverage Δ
server/events/plan_command_runner.go 40.94% <0.00%> (-6.85%) ⬇️
server/events/unlock_command_runner.go 85.71% <0.00%> (-2.53%) ⬇️
server/events/approve_policies_command_runner.go 68.88% <0.00%> (-1.49%) ⬇️
server/events/delete_lock_command.go 75.00% <0.00%> (-0.87%) ⬇️
server/events/apply_command_runner.go 71.60% <0.00%> (-0.62%) ⬇️
server/server.go 65.14% <0.00%> (-0.55%) ⬇️
cmd/server.go 79.09% <0.00%> (ø)
server/user_config.go 100.00% <0.00%> (ø)
server/events/policy_check_command_runner.go 21.87% <0.00%> (+0.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a2153b...cc0bb8f. Read the comment docs.

1. Implement the provider registry protocol internally and deny public egress, that way you control who has write access to the registry.
1. Modify your [server-side repo configuration](https://www.runatlantis.io/docs/server-side-repo-config.html)'s `plan` step to validate against the
use of not allowed providers or data sources or PRs from not allowed users. You could also add in extra validation at this point, e.g.
requiring a "thumbs-up" on the PR before allowing the `plan` to continue.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an example of how to do this validation of providers / data sources somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure exactly. I think a regex would probably work. @nishkrishnan conftest only works on the plan output so that wouldn't work right?

Copy link
Contributor

Choose a reason for hiding this comment

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

conftest works on any structured data but the way it's been written in Atlantis is to run explicitly on the plan output. Support could probably be added to run a pre-plan conftest validation step on the terraform hcl itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello, we use this two policies to block any local-exec and null_resource:

local-exec.rego

package main

# List of disallowed provisioner types
denied_provisioners := ["local-exec"]

array_contains(arr, elem) {
  arr[_] = elem
}

# count(path)-2 selects the parent object in path example: `module.test`
module_name(path) = name {
    name := sprintf("module.%s", [path[count(path)-2]])
} else = root {
    root := "root-module"
}

# Walk the configuration looking in root module and any called module for provisioners and check against denied list.
deny[reason] {
    some path, value
    walk(input.configuration.root_module, [path, value])
    resource := value.resources[_]
    provisioner := resource.provisioners[_]
    array_contains(denied_provisioners, provisioner.type)
    module := module_name(path)
    reason := sprintf(
        "%s.%s: provisioner of type '%s' is not allowed",
        [module, resource.address, provisioner.type]
    )
}

null_resource.rego

package main

# List of disallowed resources types
denied_resources := {"null_resource"}

# all resources
resources[resource_type] = all {
    some resource_type
    denied_resources[resource_type]
    all := [ name | name:= input.resource_changes[_]; name.type == resource_type ]
}

# number of creations of resources of a given type
num_creates[resource_type] = num {
    some resource_type
    denied_resources[resource_type]
    all := resources[resource_type]
    creates := [ res | res := all[_]; res.change.actions[_] == "create" ]
    num := count(creates)
}

deny[msg] {
    num_resources := num_creates["null_resource"]

    num_resources > 0

    msg := "resource of type 'null_resource' is forbidden – just for example and testing purposes"
}

@lkysow
Copy link
Member Author

lkysow commented Apr 29, 2021

I don't want perfect to be the enemy of good here so I'm going to merge this and we can continue to iterate.

@lkysow lkysow merged commit c2f1d66 into master Apr 29, 2021
@lkysow lkysow deleted the security-docs-update branch April 29, 2021 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants