Stricter validations for centralized registry workflows#23013
Stricter validations for centralized registry workflows#23013prashantkumar1982 wants to merge 10 commits into
Conversation
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
|
|
product-security-plaid-production
Bot
requested review from
jmank88 and
justinkaseman
| // defaultTenantID is the fallback tenant numeric id for the CRE environment when none | ||
| // is configured on the job spec. It matches defaultJWTAuthJobSpecTenantID in | ||
| // core/capabilities/vault and cre-platform-graphql's account service. | ||
| const defaultTenantID uint64 = 1 |
There was a problem hiding this comment.
I think we should be very careful when using defaults - if the tenant ID ever disappears from the job spec by mistake, DONs will start rejecting requests. I would rather enforce it in the job spec if that's a possibility.
There was a problem hiding this comment.
We're using the same pattern in other vault checks too, to default to a tenantID of 1 if not provided.
My thinking is if you fail to set it, things will break, and you then must set it correctly in the right job-specs.
I could also make this an explicit failure if tenantID wasn't set. But that can only be done after we first fix all job-specs and deploy them across all environments. Otherwise this change will start breaking rollouts immediately.
| // claim without on-chain proof and therefore require independent owner<->orgID | ||
| // verification before the workflow is admitted. | ||
| func isCentralizedWorkflowSource(source string) bool { | ||
| return strings.HasPrefix(source, "grpc:") || strings.HasPrefix(source, "file:") |
There was a problem hiding this comment.
Where does this source string come from?
There was a problem hiding this comment.
Here:
And that field is set here:
I will also ask @cedric-cordenier to confirm again, that this is the right authoritative place to read the source from, and isn't spoofable.
| if orgID == "" { | ||
| // Without an orgID we cannot derive the expected owner; the resolution failure | ||
| // was already logged by fetchOrganizationID. | ||
| h.lggr.Warnw("skipping centralized workflow owner/orgID verification: no organization ID resolved", |
There was a problem hiding this comment.
Are we sure we don't want to treat this as an error instead? What happens when you can't resolve an org, do you usually just proceed?
There was a problem hiding this comment.
Yes, in my understanding too, if we cannot fetch orgID, we should likely fail even running any workflow, not just this specific check.
Because we have other sub-components depending on the orgID to be set correctly.
But not sure why the code currently is set to only warn on missing orgID.
I didn't explicitly force that for this check, because it means all workflows will start failing without orgID, which currently isn't so.
@cedric-cordenier do you know if there's a harm in enforcing strict orgID presence?
| } | ||
| derivedHex := hex.EncodeToString(derived) | ||
|
|
||
| if !strings.EqualFold(strings.TrimPrefix(strings.TrimSpace(ownerHex), "0x"), derivedHex) { |
There was a problem hiding this comment.
Recommended to use ToLower as well, in case one of the addresses turns out in checksum format, and the other one doesn't.
|
2 participants
https://smartcontract-it.atlassian.net/browse/PRIV-588