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

Verify release URLs using Trusted Publisher information #16205

Merged
merged 10 commits into from
Aug 12, 2024

Conversation

facutuesca
Copy link
Contributor

@facutuesca facutuesca commented Jul 2, 2024

This PR builds on top of the work in #15891. The goal is to:

  • Add a new verified boolean field to each ReleaseURL that is set to True if the URL matches the Trusted Publisher URL
    • For example, a Source URL of github.com/user/project should appear as verified if the release was uploaded using a GitHub Actions Trusted Publisher configured with github.com/user/project as its identity.
  • Verify the URLs of a release when a new file is uploaded
    • TODO: The current implementation only marks a URL as verified during the first file upload of a release (when the release is created). Change this so that subsequent uploads can also mark URLs as verified.

Closes #15891.

@di
Copy link
Member

di commented Jul 2, 2024

TODO: The current implementation only marks a URL as verified during the first file upload of a release (when the release is created). Change this so that subsequent uploads can also mark URLs as verified.

I think this is probably fine as-is for now, as it will cover the majority of use cases, but once we support verifying URLs when attestations are uploaded we should revisit this to re-verify URLs when a file is added to a release (and, likely, add any additional URLs that were not present on the first file, if there are any).

@facutuesca
Copy link
Contributor Author

facutuesca commented Jul 2, 2024

once we support verifying URLs when attestations are uploaded

I thought the URL verification would always be via Trusted Publisher, how do the attestations fit here? From a conversation: We'll also verify URLs when uploading non-publish attestations that are not necessarily uploaded via Trusted Publishing

@di
Copy link
Member

di commented Jul 2, 2024

In the future we might have attestations that are not related to a publish that could add additional verification, but I think this is an edge case we can consider later. If we want to include updates here to handle verification for subsequent file uploads that would be fine too!

@facutuesca
Copy link
Contributor Author

facutuesca commented Jul 8, 2024

@di For verifying URLs of existing releases (when the user uploads files that are not the first file of that release), do we want to verify the URLs using the Trusted Publisher iff the file is successfully uploaded?

The alternative would be to also allow marking URLs as verified for any Trusted Publishing upload attempt that is correctly authenticated, even if the file ends up not being successfully uploaded.

@di
Copy link
Member

di commented Jul 8, 2024

I think only a successful upload makes sense. There are many reasons why an upload would fail, I don't think it's necessary to tease apart which ones would still make verification valid and which ones might not.

javanlacerda and others added 3 commits July 19, 2024 17:58
Signed-off-by: Javan lacerda <javanlacerda@google.com>
Move verification to its own function, and make the verification
checks more exhaustive by parsing the URL and adding more test
cases.
@facutuesca
Copy link
Contributor Author

I pushed a commit for verifying also URLs of existing releases. Now, if a file is uploaded for an existing release, we update the verified status of the release's URLs if they were not verified but the current file upload did verify them.

@facutuesca facutuesca marked this pull request as ready for review July 19, 2024 16:03
@facutuesca facutuesca requested a review from a team as a code owner July 19, 2024 16:03
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @facutuesca! We should follow this up with the appropriate project/release view changes 🙂

warehouse/forklift/legacy.py Outdated Show resolved Hide resolved
@miketheman
Copy link
Member

This changeset appears to have introduced some warnings in the test suite:

tests/unit/forklift/test_legacy.py::TestFileUpload::test_new_release_url_verified[https://github.com/foo-False]
tests/unit/forklift/test_legacy.py::TestFileUpload::test_new_release_url_verified[https://github.com/foo/bar/-True]
tests/unit/forklift/test_legacy.py::TestFileUpload::test_new_release_url_verified[https://github.com/foo/bar-True]
  /opt/warehouse/src/tests/unit/forklift/test_legacy.py:3906: SAWarning: SELECT statement has a cartesian product between FROM element(s) "releases" and FROM element "release_urls".  Apply join condition(s) between each element to resolve.
    db_request.db.query(ReleaseURL).filter(Release.project == project).one()

tests/unit/forklift/test_legacy.py::TestFileUpload::test_new_release_url_verified[https://google.com-False]
  /opt/warehouse/src/tests/unit/forklift/test_legacy.py:3906: SAWarning: SELECT statement has a cartesian product between FROM element(s) "release_urls" and FROM element "releases".  Apply join condition(s) between each element to resolve.
    db_request.db.query(ReleaseURL).filter(Release.project == project).one()

tests/unit/forklift/test_legacy.py::TestFileUpload::test_new_publisher_verifies_existing_release_url
  /opt/warehouse/src/tests/unit/forklift/test_legacy.py:3984: SAWarning: SELECT statement has a cartesian product between FROM element(s) "releases" and FROM element "release_urls".  Apply join condition(s) between each element to resolve.
    db_request.db.query(ReleaseURL).filter(Release.project == project).all()

Curious - why would these query not leverage the relationship?

db_request.db.query(ReleaseURL).filter(Release.project == project).one()

db_request.db.query(ReleaseURL).filter(Release.project == project).all()

@facutuesca
Copy link
Contributor Author

This changeset appears to have introduced some warnings in the test suite:

Looking into it now

@facutuesca
Copy link
Contributor Author

This changeset appears to have introduced some warnings in the test suite:

PR open to fix it here: #16528

@twm
Copy link
Contributor

twm commented Aug 20, 2024

PR to prevent recurrence here: #16529

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants