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

Add CNB Service Bindings to Image and Build resources #371

Merged
merged 8 commits into from
May 11, 2020

Conversation

scothis
Copy link
Contributor

@scothis scothis commented Apr 30, 2020

The CNB Service Bindings spec provides a structured way to expose
service metadata and credentials into a build. For example, the build
can use the binding information to install a database driver, configure
an APM agent for the built image, or discover any other capability
outside of the built container.

Each binding reference has a name, a reference to a k8s ConfigMap
holding service metadata, and optionally a k8s Secret holding service
credentials. Builds generally should not have access to the credentials
so that the values are not backed into the built image, but instead
bound fresh at runtime.

For images, the bindings are located at .spec.build.bindings:

kind: Image
...
spec:
  ...
  build:
    bindings:
    - name: my-binding
      metadataRef:
        name: provider-binding-metadata
      secretRef:
        name: provider-binding-secret

For builds, the bindings are located at .spec.bindings:

kind: Build
...
spec:
  ...
  bindings:
  - name: my-binding
    metadataRef:
      name: provider-binding-metadata
    secretRef:
      name: provider-binding-secret

In the build the bindings appear as a directory tree under the path
specified by the $CNB_BINDINGS env var. Each binding is a directory,
which in turn has a metadata directory with files and if credentials
are exposed a secret directory with more files. The environment
variable and volume mounts are available during the detect and build
steps. Currently, the value of $CNB_BINDINGS is /var/bindings but
should not be presumed to be stable or consistent with the value at
runtime.

There is no guarantee the same services, or types of services, will be
bound at runtime.

Refs #369

The CNB Service Bindings spec provides a structured way to expose
service metadata and credentials into a build. For example, the build
can use the binding information to install a database driver, configure
an APM agent for the built image, or discover any other capability
outside of the built container.

Each binding reference has a name, a reference to a k8s ConfigMap
holding service metadata, and optionally a k8s Secret holding service
credentials. Builds generally should not have access to the credentials
so that the values are not backed into the built image, but instead
bound fresh at runtime.

For images, the bindings are located at `.spec.build.bindings`:

```
kind: Image
...
spec:
  ...
  build:
    bindings:
    - name: my-binding
      metadataRef:
        name: provider-binding-metadata
      secretRef:
        name: provider-binding-secret
```

For builds, the bindings are located at `.spec.bindings`:

```
kind: Build
...
spec:
  ...
  bindings:
  - name: my-binding
    metadataRef:
      name: provider-binding-metadata
    secretRef:
      name: provider-binding-secret
```

In the build the bindings appear as a directory tree under the path
specified by the `$CNB_BINDINGS` env var. Each binding is a directory,
which in turn has a `metadata` directory with files and if credentials
are exposed a `secret` directory with more files. The environment
variable and volume mounts are available during the detect and build
steps. Currently, the value of `$CNB_BINDINGS` is `/var/bindings` but
should not be presumed to be stable or consistent with the value at
runtime.

There is no guarantee the same services, or types of services, will be
bound at runtime.

Refs buildpacks-community#369
@matthewmcnew
Copy link
Collaborator

This is so good @scothis!

I'd like to take to a moment to think through the security implications of allowing arbitrary secrets to be mounted in the build pod as you aptly noted in #369.

@codecov-io
Copy link

codecov-io commented May 5, 2020

Codecov Report

Merging #371 into master will increase coverage by 0.73%.
The diff coverage is 92.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #371      +/-   ##
==========================================
+ Coverage   67.52%   68.26%   +0.73%     
==========================================
  Files          81       81              
  Lines        3307     3409     +102     
==========================================
+ Hits         2233     2327      +94     
- Misses        800      805       +5     
- Partials      274      277       +3     
Impacted Files Coverage Δ
pkg/apis/build/v1alpha1/build.go 43.93% <0.00%> (-1.38%) ⬇️
pkg/apis/build/v1alpha1/image_types.go 50.00% <ø> (ø)
pkg/apis/build/v1alpha1/image_validation.go 96.72% <66.66%> (-3.28%) ⬇️
pkg/buildpod/generator.go 64.86% <78.94%> (+4.86%) ⬆️
pkg/apis/build/v1alpha1/build_pod.go 97.55% <100.00%> (+0.31%) ⬆️
pkg/apis/build/v1alpha1/build_validation.go 88.70% <100.00%> (+9.92%) ⬆️
pkg/apis/build/v1alpha1/image_builds.go 63.70% <100.00%> (+1.39%) ⬆️

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 019fd44...d196f92. Read the comment docs.

Copy link

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

This is awesome. Thanks @scothis

@scothis scothis marked this pull request as ready for review May 6, 2020 15:54
scothis added 2 commits May 6, 2020 12:04
There are questions that need to be further explored about the
implications of supporting secret binding. Exposing the secret name
would potentially expose an enumeration attack again other secrets in
the namespace.
@scothis
Copy link
Contributor Author

scothis commented May 7, 2020

Used the validator to disable the secretRef on the binding until we have a better understanding of how to protect again secret enumeration attacks, or direct capturing of secrets if they know the name.

@scothis
Copy link
Contributor Author

scothis commented May 8, 2020

Based on our conversation yesterday, I switch up the secret handling to forbid binding secrets that are attached to any service account in the namespace. This should offer a reasonable compromise to protect registry, and api server credentials while still allowing bindings to work as expected.

Users who need more fine grain restrictions should look at applying a policy engine like OPA to make those access decisions.

Copy link

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

This looks very excellent. Thanks @scothis !!

namespace: namespace,
bindings: []v1alpha1.Binding{
{
Name: "naughty",

Choose a reason for hiding this comment

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

lolol

@@ -0,0 +1,64 @@
apiVersion: build.pivotal.io/v1alpha1

Choose a reason for hiding this comment

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

Love the samples!!!

@@ -110,6 +110,51 @@ func (b *Build) BuildPod(config BuildPodImages, secrets []corev1.Secret, bc Buil
SubPath: b.Spec.Source.SubPath, // empty string is a nop
}

bindingVolumes := []corev1.Volume{}

Choose a reason for hiding this comment

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

can this be moved into a private method setupBindings()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

@matthewmcnew matthewmcnew left a comment

Choose a reason for hiding this comment

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

🎈 This is great! Thanks @scothis!

@scothis scothis requested a review from thisisnotashwin May 11, 2020 15:07
Copy link

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

Thanks @scothis !!!

@thisisnotashwin thisisnotashwin merged commit 60605fd into buildpacks-community:master May 11, 2020
@scothis scothis deleted the cnb-bindings branch May 11, 2020 16:07
matthewmcnew added a commit that referenced this pull request May 12, 2020
- #371 will list all service accounts on build create
matthewmcnew added a commit that referenced this pull request May 12, 2020
- #371 will list all service accounts on build create
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.

5 participants