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

Initial Auditing Configuration and Usage of the kubernetes.io GCP Organization #213

Merged
merged 7 commits into from
Oct 29, 2019

Conversation

hh
Copy link
Member

@hh hh commented Apr 3, 2019

As an community security auditor
I want to retrieve the iam policies and permissions for the kubernetes.io GCP organisation
In order to  provide transparency into who can access and configure community maintained resources

Given an gcloud account within the k8s-infra-gcp-auditors google group
When ./audit.sh is run and a PR created with the resulting policy changes
Then iam configuration can be peered reviewed / audited

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 3, 2019
@k8s-ci-robot k8s-ci-robot requested a review from cblecker April 3, 2019 08:36
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 3, 2019
@k8s-ci-robot k8s-ci-robot requested a review from nikhita April 3, 2019 08:36
@hh
Copy link
Member Author

hh commented Apr 3, 2019

Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Great starting point

audit/README.md Outdated Show resolved Hide resolved
audit/README.md Show resolved Hide resolved
audit/README.md Outdated Show resolved Hide resolved
audit/README.md Show resolved Hide resolved
* Audit report

How to automate:
* How do we audit for iam changes as they happen, rather than polling
Copy link
Member

Choose a reason for hiding this comment

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

We could (once this is working as we want and once we have our cluster up) run this as a CronJob daily or even hourly, posting any deltas to slack or something

audit/audit.sh Outdated
do
gcloud organizations get-iam-policy $CNCF_GCP_ORG --format=$format \
> cncf-org-policy.$format
gcloud projects get-iam-policy kubernetes-public --format=$format \
Copy link
Member

Choose a reason for hiding this comment

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

Rather than "kubernetes-public" shouldn't we be doing a loop over all projects?

e.g.

gcloud projects list \
    --filter "parent.id=758905017065" \
    --format "value(name, projectNumber)" \
    | while read NAME NUM; do \
        gcloud projects get-iam-policy $NAME --format=$format > $NAME.policy.$format
        gcloud iam roles list --project=$NAME --format=$format > $NAME.roles.$format
    done

Copy link
Member

Choose a reason for hiding this comment

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

Also, I vote for JSON - much easier to visually verify

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently doing json and yaml, I can remove yaml if you like.

audit/audit.sh Outdated Show resolved Hide resolved
audit/audit.sh Outdated Show resolved Hide resolved
audit/audit.sh Outdated
mkdir -p buckets
for BUCKET in `gsutil ls -p kubernetes-public | awk -F/ '{print $3}'`
do
gsutil ls -r gs://$BUCKET/ > buckets/$BUCKET.txt
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to audit the contents of buckets, or just permissions? This will trigger a lot of updates -- e.g. billing is dumped at least daily.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just permissions are fine, I think there are object/file/folder specific permissions that can be set.
We may need a way to capture these or set a remove the ability to set per file perms and only have bucket level.
Thoughts?

audit/audit.sh Outdated Show resolved Hide resolved
@thockin
Copy link
Member

thockin commented May 10, 2019

Ping on this. As we start expanding scope, the fully formed version of this would make me feel better...

@hh hh force-pushed the cncf-ci-audit branch from d8718f6 to c4eaa12 Compare May 15, 2019 15:43
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 15, 2019
@spiffxp
Copy link
Member

spiffxp commented May 15, 2019

@thockin to take a final pass on this

@hh hh force-pushed the cncf-ci-audit branch from c4eaa12 to ca16382 Compare May 15, 2019 16:02
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 15, 2019

# gcloud organizations describe $CNCF_GCP_ORG 2>&1
# ERROR: (gcloud.organizations.describe)
# User [[email protected]] does not have permission to access organization []
Copy link
Member Author

Choose a reason for hiding this comment

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

@thockin Can we add 'gcloud.organizations.describe' to the k8s-infra-gcp-auditors?

Copy link
Member

Choose a reason for hiding this comment

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

That's not the permission name. I added 'Organization Viewer' which seems to have the right permissions. Try it and let me know?

Copy link
Member

Choose a reason for hiding this comment

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

did you try this again?

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 15, 2019
@@ -0,0 +1,358 @@
---
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that the dev-cluster-turnup has a many more services enabled than our primary one.

@hh hh force-pushed the cncf-ci-audit branch from e60bda7 to 3f5ee1b Compare June 15, 2019 18:35
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 15, 2019
for service in `gcloud services list --filter state:ENABLED --format=json \
| jq -r .[].config.name | sed s:.googleapis.com::`
do
case $service in
Copy link
Member Author

Choose a reason for hiding this comment

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

We do a bit of iterating over the services. How far do we want to dig into the configuration of each.

Copy link
Member

Choose a reason for hiding this comment

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

I am less concerned about service configs than IAM and access.

Copy link
Member Author

Choose a reason for hiding this comment

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

Understood. Dumping the entire config is a bit out of scope, but I added it the documentation/TODO completeness later.

@hh hh mentioned this pull request Jun 16, 2019
@spiffxp
Copy link
Member

spiffxp commented Jun 26, 2019

/cc

@k8s-ci-robot k8s-ci-robot requested a review from spiffxp June 26, 2019 15:53
@thockin thockin self-assigned this Jun 29, 2019
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Can we get a form of this committed soon, so we can use it as we turn more things on? Then iterate..

@spiffxp
Copy link
Member

spiffxp commented Jul 10, 2019

this has grown quite a lot, do we need to open up a new PR?

@spiffxp
Copy link
Member

spiffxp commented Jul 10, 2019

@thockin @hh WDYT about merging this as-is and iterating from there?

@hh
Copy link
Member Author

hh commented Jul 10, 2019

Since adding a few new projects the case statement iterating over services has more unhandled services.

cat log.mkd | grep Unhandled | sort | uniq
# Unhandled Service admin #
# Unhandled Service bigtable #
# Unhandled Service bigtableadmin #
# Unhandled Service cloudapis #
# Unhandled Service cloudbuild #
# Unhandled Service clouddebugger #
# Unhandled Service cloudresourcemanager #
# Unhandled Service cloudscheduler #
# Unhandled Service cloudtrace #
# Unhandled Service container #
# Unhandled Service containeranalysis #
# Unhandled Service containerregistry #
# Unhandled Service containerscanning #
# Unhandled Service datastore #
# Unhandled Service groupssettings #
# Unhandled Service iam #
# Unhandled Service iamcredentials #
# Unhandled Service pubsub #
# Unhandled Service servicemanagement #
# Unhandled Service serviceusage #
# Unhandled Service sql-component #
# Unhandled Service stackdriver #

@hh
Copy link
Member Author

hh commented Jul 10, 2019

These are the new perms and the commands are commented out in audit-gcp under each TODO:

TODO: compute Needs compute.projects.get
TODO: Ensure bucket-policy-only, for simplicity in Auditing
TODO: logging needs serviceusage.services.use
TODO: monitoring needs serviceusage.services.use
TODO: storage-api needs storage.buckets.get for auditors
TODO: Verify how Big Query is configured / audited
TODO: Verify how OS Login is configured / audited

@hh
Copy link
Member Author

hh commented Jul 10, 2019

I'm keen with merging as is and evolving.

@spiffxp
Copy link
Member

spiffxp commented Jul 10, 2019

/approve
/lgtm
/hold
I'm going to leave the /hold cancel to @thockin

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2019
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spiffxp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2019
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Mostly very minor comments - would love to get this in and then do a group review of the data.

@@ -0,0 +1,100 @@
#!/bin/bash
# set -x -e
CNCF_GCP_ORG=758905017065
Copy link
Member

Choose a reason for hiding this comment

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

You could source infra/gcp/lib.sh if there are any more utilities to re-use. This is unlikely to change :)


# gcloud organizations describe $CNCF_GCP_ORG 2>&1
# ERROR: (gcloud.organizations.describe)
# User [[email protected]] does not have permission to access organization []
Copy link
Member

Choose a reason for hiding this comment

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

did you try this again?

@@ -0,0 +1,100 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

need copyright header

| while read PROJECT NUM; do \
export CLOUDSDK_CORE_PROJECT=$PROJECT
echo "### Auditing Project: ${PROJECT} ###"
mkdir -p $PROJECT
Copy link
Member

Choose a reason for hiding this comment

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

Since this assumes CWD is the script's root, maybe we should check that and print a warning if it is not?

storage-api.googleapis.com)
echo TODO: Add storage.buckets.get for auditors
echo ...to kubernetes_public_billing and any newer buckets...
echo TODO: Ensure bucket-policy-only, for simplicity in Auditing
Copy link
Member

Choose a reason for hiding this comment

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

We do set bucketpolicyonly now. We do not set logging and we do not set cors. Should we?

@@ -0,0 +1,156 @@
gs://kubernetes_public_billing/billing--2019-01-10.csv
Copy link
Member

Choose a reason for hiding this comment

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

We probably should not be dumping bucket contents or VM names or anything that is likely to be constantly changing. Unlikely that any 2 audits could produce the same data for this.

#### gsutil cors get gs://$BUCKET/
#### gsutil logging get gs://$BUCKET/
gsutil iam get gs://$BUCKET/ > $PROJECT/buckets/$BUCKET.iam.json
gsutil ls -r gs://$BUCKET/ > $PROJECT/buckets/$BUCKET.txt
Copy link
Member

Choose a reason for hiding this comment

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

We should not do this - it will be always changing

compute)
echo TODO: $service Needs compute.projects.get
#### gcloud compute project-info describe
#### gcloud compute instances list --format=$format > $PROJECT/services/compute.instances.$format
Copy link
Member

Choose a reason for hiding this comment

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

we should not list VMs - they will change as clusters get upgraded

Copy link
Member

Choose a reason for hiding this comment

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

disks are somewhat dubiously useful, too. Clusters seems OK

echo Processing: $service
mkdir -p dns
gcloud dns project-info describe $PROJECT --format=$format > $PROJECT/services/dns.info.$format
gcloud dns managed-zones list --format=$format > $PROJECT/services/dns.zones.$format
Copy link
Member

Choose a reason for hiding this comment

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

same as above - audit should catch metadata, not data

@@ -0,0 +1,33 @@
{
"bindings": [
Copy link
Member

Choose a reason for hiding this comment

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

graveyard is going away, so we can nuke all these files

@dims
Copy link
Member

dims commented Jul 24, 2019

/assign @hh

@spiffxp
Copy link
Member

spiffxp commented Aug 7, 2019

merge and iterate

@thockin
Copy link
Member

thockin commented Oct 23, 2019

Ping - I think it's back in your court, @hh

@hh
Copy link
Member Author

hh commented Oct 29, 2019

/hold

@hh
Copy link
Member Author

hh commented Oct 29, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 29, 2019
@hh
Copy link
Member Author

hh commented Oct 29, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 9d12330 into kubernetes:master Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants