Giter VIP home page Giter VIP logo

Comments (21)

MarkLodato avatar MarkLodato commented on July 25, 2024 4

Long-term, I agree that a separate "source control attestation" is likely the most robust solution. I imagine it would include not just visibility but also retention, two-person review requirements, etc. The plan is for a future SLSA Source Track to gather requirements and flesh this out.

In the meantime, a source_visibility build provenance extension seems like a good compromise. Builders who already generate provenance can fairly easily add a field indicating what they know about the source repo, if anything. Non-integrated builders like Circle and GCB can perhaps query the source host's API, if desired.

from fulcio.

kpk47 avatar kpk47 commented on July 25, 2024 3

I'm not convinced that a source_visibility field in the provenance is useful for the proposed feature. Visibility is mutable, so relying on a point-in-time snapshot may give users a false sense of the package's trustworthiness.

I think there are two slightly different problems being conflated here: whether npm should accept a package, and whether it should warn a user about a package present in the registry. A source_visibility field/attestation only solves the first problem, but your problem description makes it sound like you want to solve both.

How set are you against polling the source repo? Doing so seems much more appropriate for keeping the UI up-to-date with whether a package has public source code, and you may be able to improve reliability at upload time with an appropriate retry strategy. Or you could always use a different mechanism to solve each problem, at the cost of additional complexity.

from fulcio.

MarkLodato avatar MarkLodato commented on July 25, 2024 2

Now that I better understand the use case, I agree with @kpk47. Attestations only capture immutable claims, so the proposed extension would only be a claim that the source repo was world readable at the time of the build. But it's not clear to me how that's useful to the client.

In particular, the proposed solution has two downsides:

  • It only checks availability at the time of the build, which could be long in the past. If the client wants to know "are the sources available to me," this doesn't really tell them that answer.
  • It requires integration with the builder.

Polling by the registry seems both more accurate and easier to implement. Am I missing something?

Edit: I still think source attestations will be useful in the future, just not for mutable information like availability. ("Retention" is a bit different, where some system makes an attestation claiming that it promises to retain for some period of time.)

from fulcio.

feelepxyz avatar feelepxyz commented on July 25, 2024 2

For source_visibility, I still think we need a more well-defined specification to ensure interoperability. Specifically, GitHub and GitLab might interpret the field slightly differently, and other vendors need a way to populate it themselves.

It would be straight forward to align on the values for github/github as they are currently exactly the same but I'm also hesitant to do this as a requirement in Fulcio. We opted to not do the same for other extension values when speccing them out here.

One example is the runner_environment extension, the backing claim value for GitHub is github-hosted or self-hosted and for GitLab it's gitlab-hosted or self-hosted. Fulcio could transform these values to platform-hosted and self-hosted for consistency across providers but it opens the door for breaking changes in Fulcio from providers adding a new value to one of their claims, which might not be breaking on their end for example.

I think it would be fair to expect verifiers to have some platform specific knowledge when verifying extension values as there will always be semantics that differ.

@haydentherapper do you have any other thoughts on this?

from fulcio.

feelepxyz avatar feelepxyz commented on July 25, 2024 1

I think the Right Thing (TM) to do is, rather than cram the visibility of the repo into the build provenance, to have a separate "repo visibility attestation." This leads to easier-to-define behavior for the bait-and-switch case (delete/hide the repo right after publication).

This is very interesting and definitely something I think we should dig into more! It removes the problem of yanking provenance or anything weird like this. I would like to carefully consider the implications of this though at npm's scale, as we'd potentially end up having n-visibility attestations per package in the registry with provenance, and these signed bundles are not tiny.

There might be some common work around handling "auxiliary" attestations better, SLSA VSAs come to mind. Should we allow third-parties to attest to a package's SLSA level/source visibility? Should the ecosystem registry also create these? Where would we host the attestation and how does a verifier discover them if created by third-parties?

However, I think we don't want to let the ideal get in the way of the good here so I'm changing my mind and now think this is a reasonable request for performance/reliability reasons, especially if GitLab is willing to support this too.

My preference would be to move ahead with adding a new source_visibility extension as we can roll it out with minimal effort and then iterate from there. I'll create a new issue to discuss a separate attestation for source visibility and how this might interact with verifiers.

from fulcio.

MarkLodato avatar MarkLodato commented on July 25, 2024 1

I see. Thanks for explaining @feelepxyz and @znewman01! I thought it was an either-or situation. Doing both sounds good to me!

Let's be clear about the semantics of source_visibility. I don't know what was intended "internal" vs "private". For "public", would this be a claim that the builder was able to fetch the source without any authentication or special privileges? (Originally I thought it required a special API call to query permissions, but now I realize the builder can just try to simulate a "person on the internet" request.)

from fulcio.

feelepxyz avatar feelepxyz commented on July 25, 2024 1

Let's be clear about the semantics of source_visibility

Going to try and summarise my thinking so far to see if we're aligned here

  • Populate a source_visibility cert extension in Fulcio from the CI/CD providers OIDC ID token if it has a backing claim. Trusting what's in the ID token at the time of publish, and the builder would not need to check reachability.
  • GitHub Actions provides a repository_visibility claim and it has the values "private,internal,public" (internal being a private "inner-source" repo)
  • GitLab doesn't have a backing claim today but sounds like it will be easy for them to add a similar claim project_visibility to their token, that would have the same values as the github claim
  • The source_visibility extension is optional in Fulcio to allow CI systems to omit it if they don't have access to this info or don't want to do a reachability check.
  • On the npm side we would only validate the cert extension if it was set at publish time, allowing any certs that don't have it (e.g. if circle/buildkite can't provide a backing claim). I don't see any real issue with this as we still do the later just in time check to the actual source.
  • npm will also perform a just-in-time reachability checks to the source repository/commit when viewing a package on npmjs.com, example

from fulcio.

MarkLodato avatar MarkLodato commented on July 25, 2024 1

Thanks, @feelepxyz. The last two bullets sound good to me (npm side).

For source_visibility, I still think we need a more well-defined specification to ensure interoperability. Specifically, GitHub and GitLab might interpret the field slightly differently, and other vendors need a way to populate it themselves.

For example:

  • Fulcio specifies the meaning of the source_visibility cert extension. Strawman:
    • public means that the source (repo, ref, commit) is visible to any unauthenticated user on the internet.
    • private means that the source is not public.
    • internal means ...? (Is this needed? Presumably it has higher precedence than private?)
    • Absent means no claim about visibility.
    • (Are custom values allowed?)
  • Fulcio defines how the source_visibility is pulled from the OIDC token. Perhaps this is different for different OIDC providers, and it might not be a verbatim copy of the OIDC field (e.g. if we map both OIDC internal and private to Fulcio private.)

To be clear, I'm just trying to get agreement on the need for such a specification. I assume the actual specification would be left to some pull request.

from fulcio.

haydentherapper avatar haydentherapper commented on July 25, 2024 1

I'm supportive of adding this extension, and agree that it's expected that verifiers will have some platform knowledge.

One small comment would be to consider changing the claim name source_visibility to something that clearly denotes that this represents source visibility at the time of provenance, but I'm not opposed to leaving this as is if there's not a good option.

from fulcio.

znewman01 avatar znewman01 commented on July 25, 2024

My initial reaction to this proposal is pretty negative from an idealistic standpoint. If a verifier cares about visibility of a repo, they should just check it themselves—why would you care what someone else says about it? The exception would be the OIDC provider, who you are able to trust anyway (which is why this may be reasonable).

GitLab does not currently expose a this in claims but does in ENV in CI_PROJECT_VISIBILITY.

As you're pointing out here, this is nonstandard. Further, we don't want to just trust Cosign's word on this—we'd need GitLab to stick it in the OIDC tokens.

Ideally we would be able to enforce this by inspecting the Fulcio signing certificate, thereby not introducing any additional requests in the publish flow.

From a practical standpoint, that is a reasonable concern. But I don't think we're going to solve it across all possible forges.

So I remain weakly opposed.

from fulcio.

wlynch avatar wlynch commented on July 25, 2024

The builder isn't always the same as source provider (e.g. Circle CI, Cloud Build, etc), so this may only shift the problem to those systems, and they aren't really authoritative to say whether the source is public. 😢

Would retries on both the NPM server / client side be sufficient to work around this for transient blips?

Also to double check - this is mainly a optimization to check visibility at publish time, right? npm will still show warnings if the repo is later hidden / deleted / renamed / etc?

from fulcio.

haydentherapper avatar haydentherapper commented on July 25, 2024

Cc @laurentsimon @ianlewis

from fulcio.

marshall007 avatar marshall007 commented on July 25, 2024

As you're pointing out here, this is nonstandard. Further, we don't want to just trust Cosign's word on this—we'd need GitLab to stick it in the OIDC tokens.

@znewman01 we can easily support that on our end. I've updated the relevant issue to add support for this claim here: https://gitlab.com/gitlab-org/gitlab/-/issues/404722#note_1462094036

From a practical standpoint, that is a reasonable concern. But I don't think we're going to solve it across all possible forges.

Agreed, I think support for populating this extension should be considered optional and I don't think it makes sense to block package publication based on its value. To @wlynch's point, what is a build provider going to do when they cannot make authoritative claims about the source? By making this extension required they would be forced to hard-code some value here, which is going to be useless at best (ex. hardcoding a value like unknown) and incorrect at worst (ex. hardcoding a value like public to ensure npm publish is not blocked from their platform).

Also to double check - this is mainly a optimization to check visibility at publish time, right? npm will still show warnings if the repo is later hidden / deleted / renamed / etc?

@wlynch @feelepxyz these may be cases where might actually make sense to block package publication (i.e. require manual remediation). To phrase it differently, maybe consider blocking publication when it appears a blatant "bait and switch" has taken place? This could mean: source visibility changes from public -> private, source repo is renamed or modified, package name targeting the source repo is changed, etc.

from fulcio.

haydentherapper avatar haydentherapper commented on July 25, 2024

Also to double check - this is mainly a optimization to check visibility at publish time, right? npm will still show warnings if the repo is later hidden / deleted / renamed / etc?

I also was wondering this, if you're going to have to periodically check repo visibility. I recognize that you can handle failure more gracefully in this case since it'll be an async check.

However, this would mean trusting the artifact repository rather than provenance, which should be avoided. If the provenance stated the package source was visible at the time of publishing, I think it would be acceptable to leave the package as trustworthy. You could also publish an auditable event noting its change in visibility (something like a revocation event, but triggered by the artifact repository), but not sure if npm's package lifecycle flow has support for this.

from fulcio.

feelepxyz avatar feelepxyz commented on July 25, 2024

As you're pointing out here, this is nonstandard. Further, we don't want to just trust Cosign's word on this—we'd need GitLab to stick it in the OIDC tokens.

💯 I'm imaging this extensions would be optional and only populated from OIDC token claims if it's set. Seems like GitLab would be able to get this added.

It definitely needs to be optional for CI systems like Circle and Cloud Build and we'll need to see what these systems can provide when we get them added.

Also to double check - this is mainly a optimization to check visibility at publish time, right? npm will still show warnings if the repo is later hidden / deleted / renamed / etc?

Yes exactly, this would allow us to only check and trust what's in the Fulcio certificate at the time of publish and mark it as trusted based on this. We would just omit this check if the extension isn't set. We would still show warnings on npmjs.com if the repo is later deleted or renamed but up to the consumer to decide if they trust it based on this additional signal.

If there's a request failure/timeout we can communicate this on npmjs.com and the consumer can doublecheck themselves, whereas if we check at publish time we would need to either block the publish or worse, try and yank an already published provenance if it fails repeatedly.

I would love to avoid adding any kind of real request to the source repo during the publish request as it either means adding ~250-500ms of latency to the request or performing async retries that significantly complicates the failure case (and as Hayden pointed out, this would also mean trusting the artifact repository over the provenance).

from fulcio.

znewman01 avatar znewman01 commented on July 25, 2024

Zooming out, it seems like what we're looking for is:

  1. The artifact is built from a given repo ("build provenance attestation").
  2. That repo is public (mostly at the time of publication, but we may want to check periodically).

I think the Right Thing (TM) to do is, rather than cram the visibility of the repo into the build provenance, to have a separate "repo visibility attestation." This leads to easier-to-define behavior for the bait-and-switch case (delete/hide the repo right after publication).

(and as Hayden pointed out, this would also mean trusting the artifact repository over the provenance).

IMO that's actually what we want in a world where we've teased apart the build provenance and the source visibility claims: ideally you'd be checking the visibility from all over the internet, like ACME multi-vantage-point validation. Then you'd want clients to require "visibility attestations" from some threshold of users.


However, I think we don't want to let the ideal get in the way of the good here so I'm changing my mind and now think this is a reasonable request for performance/reliability reasons, especially if GitLab is willing to support this too.

from fulcio.

feelepxyz avatar feelepxyz commented on July 25, 2024

I think there are two slightly different problems being conflated here: whether npm should accept a package, and whether it should warn a user about a package present in the registry. A source_visibility field/attestation only solves the first problem, but your problem description makes it sound like you want to solve both.

@kpk47 great callout and this wasn't very clear. I was only thinking of solving the first issue with the source_visibility extension/field. I can definitely see how this isn't ideal as it means the extension value is only useful shortly after the certificate has been minted, and for npm it would only be used to perform validation at publish time, so not meant for downstream validators after the fact.

For the UI we'll be doing a just in time check to the actual source repository, and potentially cache this response for some time on the registry. Here's an example of a package that has an unreachable source.

At publish time I would like to block publishing from private sources to avoid maintainers getting into a known bad state. This extension would allow us to do this check without issuing a real source request and avoid having to deal with the edge cases around request failures or timeouts.

The options I can think of for this publish time check:

  • Add a new source_visibility extension to the fulcio cert and gate based on this at publish time
  • Synchronously perform a request to the source at publish time and block publish if the request 404s or fails/times out (failure cases will be annoying as the the only way to fix it is to retry the workflow and hope it works)
  • Synchronously perform a request to the source at publish time and block publish if the request 404s and let it through on any non-404 response code (allowing failures/timeouts to go through)
    • Haven't thought through all the implications here but this would probably be my preferred option in case it doesn't make sense to add a source_visibility extension to the fulcio cert
  • Asynchronously perform a request to the source and yank provenance after the fact (this would allow us to retry the request but this seems worse for downstream verifiers that e.g. expect a certain package to always have provenance)
  • Don't check at publish time and just show the UI warning (would like to avoid this as it means we don't tell users when they are entering into a known bad state)

from fulcio.

znewman01 avatar znewman01 commented on July 25, 2024

Attestations only capture immutable claims, so the proposed extension would only be a claim that the source repo was world readable at the time of the build. But it's not clear to me how that's useful to the client.

Agreed—but I think "was readable at time X" is exactly what we want here. For the initial publication, npm (and users) should be checking that the package was readable when published; missing the initial-publication-visibility-attestation should be an error. Later on, we want periodic checks. So the lack of an recent (within time T) visibility attestation should result in a warning.

from fulcio.

feelepxyz avatar feelepxyz commented on July 25, 2024

changing the claim name source_visibility to something that clearly denotes that this represents source visibility at the time of provenance

Yes thoughts on any of the following?

  • source_visibility_at_signing
  • source_visibility_at_sign
  • source_visibility_at_build
  • source_visibility_snapshot

from fulcio.

feelepxyz avatar feelepxyz commented on July 25, 2024

Looking at the the existing claims, the source ones are all prefixed with Source repository so we should follow this pattern: Source repository visibility.

Source repository visibility at sign etc feels a bit long! I wonder if it's enough to only mention the just in time aspect in the documentation?

from fulcio.

feelepxyz avatar feelepxyz commented on July 25, 2024

Went with "Source repository visibility at signing" for now: #1279

from fulcio.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.