Conversation
There was a problem hiding this comment.
Q:
when comparing with GH form, there looks like there's some more stuff we could do.
eg; they have a call to hit the api to lookup the owner name. We could do some stuff like that, hit CCI API to fetch project name, org names, but not sure its worth it? We could silently fail if its not a OSS project. (if we get auth rejected)
Also, I can see the GH form has provision for API key, but I'm not yet clear where the user would provide that? not seeing it in the warehouse ui.
There was a problem hiding this comment.
so I think will do a follow up pr around adding something similar to look up a project by uuid and fill in some other info (when its a public project). let me know if you think its a bad idea to make it a follow up pr as opposed to taking care of it now? fwict it will be a simple migration to add a few more fields
There was a problem hiding this comment.
We do this to prevent resurrection attacks (i.e., a new GitHub repo created with the same owner & repository name would have a different repository ID). If CircleCI permits re-using the unique fields being used for the publisher here, then we should do the same thing.
I think we should just do that as part of this PR, otherwise we will end up in a state where we have IDs for new projects going forwards but not ones added before the follow-up PR.
The API key is not the user's key, but our key for making these requests to the GitHub API.
| {% macro circleci_form(request, pending_circleci_publisher_form) %} | ||
| <p> | ||
| {% trans href="https://circleci.com/docs/openid-connect-tokens/" %} | ||
| Read more about CircleCI's OpenID Connect support <a href="{{ href }}">here</a>. |
There was a problem hiding this comment.
todo: prep page about this form for CCI guides, lets add a link to that as well. though its a chicken egg issue i suspect. but if we can get this deployed to staging warehouse, can then build a guide and do a followup pr to add the link before hitting prod
| class_="form-group__field", | ||
| aria_describedby="circleci_org_id-errors") | ||
| }} | ||
| <p class="form-group__help-text">{% trans %}The CircleCI organization ID (UUID) that owns the project{% endtrans %}</p> |
There was a problem hiding this comment.
do we need more guidance here where to find this info? i am leaning to no, since we will have a guide we can link to in cci docs that can go in depth
There was a problem hiding this comment.
I also noticed theres space for a guide on pypi docs as well, so between those 2 it should be sufficient
There was a problem hiding this comment.
Is this something users are aware of? Or should we be getting it via API based on the organization name?
warehouse/migrations/versions/549bdbefa6bd_add_circleci_oidc_publisher.py
Outdated
Show resolved
Hide resolved
warehouse/migrations/versions/549bdbefa6bd_add_circleci_oidc_publisher.py
Outdated
Show resolved
Hide resolved
a2568a4 to
44d7223
Compare
|
milestone: the 0.0.1.dev87 was published from to my local pypi
|
6ec2327 to
9fe9b93
Compare
5b980b7 to
459fa6e
Compare
459fa6e to
669a67a
Compare
Add CircleCIPublisher and PendingCircleCIPublisher models following the existing OIDC publisher pattern. CircleCI identifies projects by circleci_org_id and circleci_project_id (UUIDs from OIDC claims). - CircleCIPublisherMixin with shared logic - CircleCIPublisher for active publishers - PendingCircleCIPublisher for pending registration - Export models and issuer URL from __init__.py - Register in OIDC_PUBLISHER_CLASSES, OIDC_ISSUER_SERVICE_NAMES, and OIDC_ISSUER_ADMIN_FLAGS in utils.py Note: Column names use circleci_ prefix to avoid collision with PendingOIDCPublisher.organization_id (PyPI org FK). Amp-Thread-ID: https://ampcode.com/threads/T-019bb846-115f-75e8-9c39-9694e10df5da Co-authored-by: Amp <amp@ampcode.com>
- Add DISALLOW_CIRCLECI_OIDC admin flag for killswitch control - Register CircleCI OIDC publisher service factory Amp-Thread-ID: https://ampcode.com/threads/T-019bb846-115f-75e8-9c39-9694e10df5da Co-authored-by: Amp <amp@ampcode.com>
- CircleCIPublisherBase with circleci_org_id and circleci_project_id fields - CircleCIPublisherForm for project-level publisher management - PendingCircleCIPublisherForm for pending publisher registration - Export forms from __init__.py Amp-Thread-ID: https://ampcode.com/threads/T-019bb846-115f-75e8-9c39-9694e10df5da Co-authored-by: Amp <amp@ampcode.com>
Add add_circleci_oidc_publisher view to ManageOIDCPublisherViews for registering CircleCI trusted publishers on projects. Amp-Thread-ID: https://ampcode.com/threads/T-019bb846-115f-75e8-9c39-9694e10df5da Co-authored-by: Amp <amp@ampcode.com>
Add CircleCI support to pending publisher management in account views for registering publishers before project creation. Amp-Thread-ID: https://ampcode.com/threads/T-019bb846-115f-75e8-9c39-9694e10df5da Co-authored-by: Amp <amp@ampcode.com>
Add CircleCI support to organization-level pending publisher management. Amp-Thread-ID: https://ampcode.com/threads/T-019bb846-115f-75e8-9c39-9694e10df5da Co-authored-by: Amp <amp@ampcode.com>
Add CircleCI publisher form and display sections to: - Project publishing template - Account publishing template - Organization publishing template - Base manage template (nav update if any) Amp-Thread-ID: https://ampcode.com/threads/T-019bb846-115f-75e8-9c39-9694e10df5da Co-authored-by: Amp <amp@ampcode.com>
Create circleci_oidc_publishers and pending_circleci_oidc_publishers tables with circleci_org_id and circleci_project_id columns. Amp-Thread-ID: https://ampcode.com/threads/T-019bb846-115f-75e8-9c39-9694e10df5da Co-authored-by: Amp <amp@ampcode.com>
- CircleCIPublisherFactory and PendingCircleCIPublisherFactory - Model tests for CircleCIPublisher and PendingCircleCIPublisher - Form validation tests for CircleCI publisher forms - View tests for project, account, and organization publishers Amp-Thread-ID: https://ampcode.com/threads/T-019bb846-115f-75e8-9c39-9694e10df5da Co-authored-by: Amp <amp@ampcode.com>
Add wtforms.validators.UUID to all CircleCI OIDC publisher form fields: - circleci_org_id (required) - circleci_project_id (required) - pipeline_definition_id (required) - context_id (optional) This ensures users submit valid UUIDs matching CircleCI's identifier format. Also update tests to: - Include pipeline_definition_id in test data (now required) - Add test for optional context_id with valid UUID - Add test for missing pipeline_definition_id - Add parametrized test for invalid UUID on required fields - Add test for invalid context_id UUID
Add support for two optional VCS claims in CircleCI OIDC publishers: - vcs_ref: git ref like 'refs/heads/main' - vcs_origin: VCS origin like 'github.com/organization-123/repo-1' These claims allow users to constrain publishers to specific branches or repositories for additional security. Changes: - Add model columns vcs_ref and vcs_origin (nullable) - Move claims from __unchecked_claims__ to __optional_verifiable_claims__ - Add _check_optional_string helper for simple string matching - Update __getattr__ to map claim names to attributes - Update exists(), admin_details, unique constraints, and reify() - Add form fields for vcs_ref and vcs_origin - Update all views that create CircleCI publishers
Update the CircleCI OIDC publisher migration to include: - vcs_ref column (nullable) - vcs_origin column (nullable) - Updated unique constraints to include both fields
Update test data in test_oidc_publishers.py to use valid UUID format for CircleCI fields (circleci_org_id, circleci_project_id, pipeline_definition_id) and include all required form fields (context_id, vcs_ref, vcs_origin). This ensures test data matches the UUID validation requirements added to the CircleCI form fields.
Add pipeline_definition_id, context_id, vcs_ref, and vcs_origin to the mocked CircleCI form in test_organizations.py to match the complete form field requirements.
Add tests for the optional vcs_ref and vcs_origin fields: - test_validate_with_optional_vcs_ref - test_validate_with_optional_vcs_origin - test_validate_with_all_optional_fields
Add comprehensive tests for the new optional VCS claims: - TestCheckOptionalString: Tests for the _check_optional_string helper - Empty ground truth always passes - Required value fails without claim - Required value matches/doesn't match claim - TestCircleCIPublisher updates: - test_admin_details_with_vcs_ref - test_admin_details_with_vcs_origin - test_admin_details_with_all_optional_fields - Updated test_getattr_maps_claims_to_attributes - TestPendingCircleCIPublisher updates: - Updated test_reify_creates_publisher with vcs fields - Updated test_reify_returns_existing_publisher with vcs fields Also update new_signed_claims helper to accept vcs_ref and vcs_origin.
Add the optional VCS fields to all three CircleCI publisher templates: - manage/project/publishing.html - manage/account/publishing.html - manage/organization/publishing.html Each field includes: - Label with (optional) indicator - Placeholder text showing expected format - Help text explaining the field's purpose Amp-Thread-ID: https://ampcode.com/threads/T-019be17b-aad4-7691-8ab4-01828eebb650 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019be17b-aad4-7691-8ab4-01828eebb650 Co-authored-by: Amp <amp@ampcode.com>
Regenerate migration with correct down_revision (31ac9b5e1e8b) to chain after upstream 'Add ADMINISTRATIVE to BanReason enum' migration. Amp-Thread-ID: https://ampcode.com/threads/T-019bfd26-1169-72d6-81c6-b682c6daea4d Co-authored-by: Amp <amp@ampcode.com>
Add the attestation_identity property to CircleCIPublisherMixin, following the pattern established by GitHub, GitLab, and Google publishers. Currently returns None as pypi-attestations library does not yet have a CircleCIPublisher identity class. Fulcio already supports CircleCI OIDC tokens, so this prepares the infrastructure for when upstream support is added. Refs: sigstore/fulcio#591 Amp-Thread-ID: https://ampcode.com/threads/T-019bfff2-9b8a-7199-ab46-631fcb14f9d3 Co-authored-by: Amp <amp@ampcode.com>
Add tests for parity with GitHub/GitLab model tests: - all_known_claims() assertion for claim set stability - Missing required claims raise KeyError - DB uniqueness constraint (UniqueViolation) test - verify_url() returns False for CircleCI - Multi-constraint lookup tests (vcs_ref, vcs_origin) - Exact InvalidPublisherError message test - Parametrized optional claim verification tests Signed-off-by: meeech <4623+meeech@users.noreply.github.com> Amp-Thread-ID: https://ampcode.com/threads/T-019c003a-2977-70f9-ad05-cce7fb3236a0 Co-authored-by: Amp <amp@ampcode.com>
Add tests for parity with GitHub/GitLab form tests: - ProjectNameUnavailable*Error variants (Invalid, Existing, Stdlib, Prohibited, Similar) - Optional field consistency tests for vcs_ref, vcs_origin, context_id - Parametrized invalid field tests for required UUID fields - Empty string vs None consistency for optional fields Signed-off-by: meeech <4623+meeech@users.noreply.github.com> Amp-Thread-ID: https://ampcode.com/threads/T-019c003a-2977-70f9-ad05-cce7fb3236a0 Co-authored-by: Amp <amp@ampcode.com>
Fill in actual values for optional CircleCI fields (context_id, vcs_ref, vcs_origin) to match the pattern used by GitHub/GitLab tests which populate their optional fields with real values. Signed-off-by: meeech <4623+meeech@users.noreply.github.com> Amp-Thread-ID: https://ampcode.com/threads/T-019c003a-2977-70f9-ad05-cce7fb3236a0 Co-authored-by: Amp <amp@ampcode.com>
Add pipeline_definition_id, context_id, vcs_ref, and vcs_origin to the CircleCI trusted publisher display in manage_base.html. Also update test factories to include vcs_ref and vcs_origin fields. Amp-Thread-ID: https://ampcode.com/threads/T-019c2939-421d-757c-9608-2f7b1758b9c0 Co-authored-by: Amp <amp@ampcode.com>
ran make resetdb and make initdb, seems fine
1e6acfb to
bdae7b2
Compare
|
So I think this is ready for review. I left some open questions about process - eg: would it be preferred for me to add in the attestation work into this pr? or better to leave stacked. |
di
left a comment
There was a problem hiding this comment.
FYI, looks like translations need updated and also the "revises" field for the database migration as we've had additional migrations since you generated it.
| context_id = form.context_id.data or "" | ||
| vcs_ref = form.vcs_ref.data or "" | ||
| vcs_origin = form.vcs_origin.data or "" |
There was a problem hiding this comment.
I think these and other instances of or "" should just be changed to use None? Is there a reason this must be a string and not a nullable field?
There was a problem hiding this comment.
We do this to prevent resurrection attacks (i.e., a new GitHub repo created with the same owner & repository name would have a different repository ID). If CircleCI permits re-using the unique fields being used for the publisher here, then we should do the same thing.
I think we should just do that as part of this PR, otherwise we will end up in a state where we have IDs for new projects going forwards but not ones added before the follow-up PR.
The API key is not the user's key, but our key for making these requests to the GitHub API.
| class_="form-group__field", | ||
| aria_describedby="circleci_org_id-errors") | ||
| }} | ||
| <p class="form-group__help-text">{% trans %}The CircleCI organization ID (UUID) that owns the project{% endtrans %}</p> |
There was a problem hiding this comment.
Is this something users are aware of? Or should we be getting it via API based on the organization name?


#13888
Related PRs:
pypi/pypi-attestations#166 Add CircleCI to pypi-attestations
meeech#1 Stacked pr off this one - integrates attestations. Question: would it be better to just merge this into have the one pr? I was trying to be mindful of making things easier to review.
I have tried to keep commits bite sized. I am open to split this up into multiple prs if that preferred (though not sure where the 'fault lines' would be for this since its all a bit related - forms, model, etc? let me know
MUST
while i worked with a LLM to generate this PR, i (the human) have reviewed all this code. (amp for the curious)
tbf it did a good job doing all the boilerplate.
Basically I try to adhere to https://github.com/ghostty-org/ghostty/blob/main/AI_POLICY.md