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

Adding Anchore security checks #25

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Adding Anchore security checks #25

wants to merge 15 commits into from

Conversation

denismakogon
Copy link
Member

We need to make CLI be capable to offer various Node runtimes:

  • "8"
  • "10"
  • "11"

This PR adds multistage base images:

  • 8 and 8-dev
  • 10 and 10-dev
  • 11 and 11-dev

Related to: fnproject/cli#553

@denismakogon
Copy link
Member Author

How to test this PR:

  1. Build two images {node-version} and {node-version}-dev.
  2. Create a sample function.
  3. Go to func.yaml and add build_image: ... and run_image: ... to that.
  4. Deploy a function.
  5. Run it.

@denismakogon
Copy link
Member Author

The same thing, Node images belongs here.

@denismakogon
Copy link
Member Author

One of the reasons for this change is that we havefnproject/node, which node version is totally unclear unless you'd actually investigate it. So, for node runtimes, it does matter to have explicit versioning.

@denismakogon denismakogon changed the title Making a difference between NodeJS runtimes Adding Snyk security checks Apr 18, 2019
@denismakogon
Copy link
Member Author

denismakogon commented Apr 18, 2019

This change is similar to fnproject/fdk-python#77

Some work necessary to be done:

  • add Snyk token into CI config
  • add Docker user/pass into CI config

@denismakogon denismakogon force-pushed the runtime_images branch 5 times, most recently from 64fff84 to ea5a877 Compare April 19, 2019 15:23
@denismakogon denismakogon changed the title Adding Snyk security checks Adding Anchore security checks Apr 19, 2019
@denismakogon
Copy link
Member Author

Blocked-By: anchore/ci-tools#10

@denismakogon
Copy link
Member Author

denismakogon commented Apr 24, 2019

@carimura @rdallman with help of @Btodhunter we now have a new Ahcnore orb and faster checks running within CircleCI: 3 jobs in parallel, each job takes 7 mins per 2 images (dev and runtime).
I'm going to populate this change to the rest of FDKs.

@carimura
Copy link
Member

sweet.

@rdallman
Copy link
Contributor

should we be running this on every PR? since the images are built on master and generally not touched by any PR submissions, it's not really the fault of the PR that the checks fail (usually) - we're not expecting users to fix this.

these seem like checks that should run against master and we should probably have image updates on a weekly cron or something to pick up language/image updates, since fdk's are infrequently and irregularly updated via PRs, the process as proposed here does not seem ideal at least (still OK with doing them in general, just in a different manner most likely). any thoughts?

@denismakogon
Copy link
Member Author

should we be running this on every PR? since the images are built on master and generally not touched by any PR submissions, it's not really the fault of the PR that the checks fail (usually) - we're not expecting users to fix this.

It’s true, but we have to be sure that we are safe all the time, with every PR.

these seem like checks that should run against master and we should probably have image updates on a weekly cron or something to pick up language/image updates

That’s true as well. It seems like circle ci has these options available, but still, while nightly build is a good option, having these checks on every commit still makes sense.

@rdallman
Copy link
Contributor

I think I only agree with that thesis if the PR itself changes the images. most PRs and users are not going to do this, and are not responsible for fixing those builds (arguably, the point of CI on PRs).

I see 2 things:

  • we should have a regularly scheduled image update
  • on any commits where the images change (like in the above), we should run security checks

this is different from running against every PR, but runs against any relevant PRs. both of these are possible afaik.

@denismakogon
Copy link
Member Author

denismakogon commented Apr 24, 2019

on any commits where the images change (like in the above), we should run security checks

I see where you going, but I’m not entirely sure that’s valid case, because you have no idea when Debian maintainers will release a fix to one of their packages. So, you need to run security checks at least once in a day. Docker file may remain the same while apt-get upgrade will bring new packages that you are not aware of and this IS the case when image will change.

So, the only options that is acceptable: per-PR checks even if dockerfile remain unchanged, nightly (cron) builds.

@rdallman
Copy link
Contributor

checking on PRs seems unnecessary if we're doing nightly builds? again, most PRs are not doing anything to change the state of the images, and users are not expected to fix these.

@denismakogon
Copy link
Member Author

checking on PRs seems unnecessary if we're doing nightly builds?

You have no guarantees that at that moment when PR will appear images would be up to date.

most PRs are not doing anything to change the state of the images

It’s an assumption, you have no guarantees.

and users are not expected to fix these.

Any user’s PR must be delayed until we fix image check. Images have higher priority by default. So, users will have to wait and rebase once we will fix an image.

@rdallman
Copy link
Contributor

You have no guarantees that at that moment when PR will appear images would be up to date.

why do we care? I get the point that users could change the image and make it insecure. the reality is that we're the only ones doing that.

i guess part of my whole confusion with this movement in general to conflate images into the fdk repo is that the images have almost nothing to do with the fdk code. aside from the recent addition of the fn user (which we could even do in build stage), we're not doing anything substantial over whatever image we're building from from docker hub.

if the images existed in a repo with only images, I could kind of get running the security checks on every PR, however this repo has 2 things, code and images. i'm not sure there was ever a formally agreed upon proposal to put images into fdk repos to begin with, when we start doing things like this it seems to make a lot less sense. not to mention the hassle of propagating all of this stuff across every individual fdk repo, it kinda smells like we should take a step back.

@denismakogon
Copy link
Member Author

I tend to disagree. Initial concern was that dockers repo is a dump and cemetery for images we have since iron and the dump was still growing.

Runtime images are the artifacts to FDKs only (there’s only 1 images shared across repos: go image). So, they have to live and be tested all the time along with an FDK code.

why do we care? I get the point that users could change the image and make it insecure. the reality is that we're the only ones doing that.

Why do we care? Because we committed to deliver secure code and images. Users could make images insecure but as long as they rely on our images we must ensure them that images are up to date all the time (all security issues resolved if possible).

i'm not sure there was ever a formally agreed upon proposal to put images into fdk repos to begin with

Well, I saw zero objections when we merged images to Java FDK, Python FDK and Go FDK. So, there are no technical or any other reasons for not keeping images where they actually belong.

@rdallman
Copy link
Contributor

I tend to disagree. Initial concern was that dockers repo is a dump and cemetery for images we have since iron and the dump was still growing.

sure, but it doesn't have to be. it doesn't have to be anything. we can create a new repo, etc.

Why do we care? Because we committed to deliver secure code and images. Users could make images insecure but as long as they rely on our images we must ensure them that images are up to date all the time (all security issues resolved if possible).

yes, but merging code changes does not mean we are delivering the image. I should have made this more clear. even if we merge something that breaks it, we don't have to deploy that [image] just because it hit master. point being, these are separate.

Runtime images are the artifacts to FDKs only (there’s only 1 images shared across repos: go image).

the runtime images do not even contain the FDK code, I realize that our verbiage is the source of confusion here. FDK [code] and runtime image are separate things.

point stands that we have never had a formal discussion or proposal about how to manage the images, it's been done differently for different fdks since this project started, without any discussion really. now that we're deficient and adding a significant amount of process and needing to support these better, it seems wise to have this discussion to take into account all of the tradeoffs before making a decision.

@carimura
Copy link
Member

maybe discuss IRL in next community call? then we can figure out how to move forward... or slack anytime.

@denismakogon
Copy link
Member Author

Since PR to FDK Python was merged I'll duplicate all changes here for the consistency.

 we need to make CLI be capable to offer various Node runtimes:
   - 8
   - 10
   - 11

 This PR adds multistage base images:
   - 8 and 8-dev
   - 10 and 10-dev
   - 11 and 11-dev
 all runtimes and dev images would be inspected during each build
 There's a known issue of Anchore CI tools.
 Therefore there's a need to define 1 job per 1 build+check
@denismakogon denismakogon force-pushed the runtime_images branch 2 times, most recently from 2054187 to 318f390 Compare June 4, 2019 17:35
@denismakogon
Copy link
Member Author

@crush-157 can you please help to figure out what's wrong with tests?
here's the log from the build:

TAP version 13
# print logFramer
(node:120) UnhandledPromiseRejectionWarning: TypeError: console.warn is not a function
    at /home/circleci/fdk-node/fn-fdk.js:295:17
    at processTicksAndRejections (internal/process/task_queues.js:89:5)
(node:120) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)
(node:120) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

@denismakogon denismakogon force-pushed the runtime_images branch 2 times, most recently from c7f33d5 to 6ccc7a5 Compare June 4, 2019 17:42
@denismakogon
Copy link
Member Author

Okay, nwm, i figured it out, it's all about Node.JS version, it seems like FDK requires at least Node 9. (cc @rdallman).

@denismakogon denismakogon force-pushed the runtime_images branch 2 times, most recently from 034370e to 52376b0 Compare June 4, 2019 17:49
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.

3 participants