Skip to content

fix(core): prevent false-positive RoleValidationError under condition… - #6029

Open
raza-khan0108 wants to merge 3 commits into
aws:masterfrom
raza-khan0108:fix/6019-conditional-scp-role-validation
Open

raza-khan0108 wants to merge 3 commits into
aws:masterfrom
raza-khan0108:fix/6019-conditional-scp-role-validation

Conversation

@raza-khan0108

Copy link
Copy Markdown

Description

Fixes #6019: prevents false-positive RoleValidationError exceptions during role resolution and validation (resolve_and_validate_role) when AWS accounts use condition-based Service Control Policies (SCPs) or condition-based IAM permissions boundaries.

Problem

sagemaker-core v2.15.0 introduced a pre-flight permission validation check (_evaluate_permissions via iam:SimulatePrincipalPolicy) that runs during high-level object construction (e.g., ModelTrainer(...), TrainDefaults.get_role()) before job submission.

Per AWS IAM Documentation, the IAM policy simulator does not evaluate conditional SCPs or permissions boundaries. When an AWS account uses condition-based SCPs (e.g., restricting regions, IP ranges, or required tags), iam:SimulatePrincipalPolicy returns:

  • EvalDecision: implicitDeny
  • OrganizationsDecisionDetail: {"AllowedByOrganizations": false} (or PermissionsBoundaryDecisionDetail: {"AllowedByPermissionsBoundary": "false"})

Because the previous implementation treated any non-allowed SimulatePrincipalPolicy verdict as a hard failure (RoleValidationError), valid roles in enterprise AWS Organizations environments were blocked from using high-level SDK v3 constructs (ModelTrainer, ModelBuilder, Pipeline).


Solution & Architectural Changes

This PR refactors the client-side role validation flow to gracefully handle simulator limitations and provides explicit opt-out controls:

1. Unverifiable Denial Detection

  • Added helper _is_unverifiable_denial(result: dict) -> bool in sagemaker.core.helper.iam_role_resolver to inspect SimulatePrincipalPolicy evaluation items.
  • If EvalDecision is implicitDeny due to AllowedByOrganizations: false or AllowedByPermissionsBoundary: false, the action is classified as unverifiable rather than definitively denied.

2. Partitioned Simulation Results & Warn-and-Proceed Behavior

  • _simulate_denied_actions(...) now returns a tuple Tuple[List[str], List[str]] containing (denied_actions, unverifiable_actions).
  • When _evaluate_permissions(...) encounters unverifiable actions (and zero definitive denied actions), it:
    1. Logs an informative WARNING message detailing that Organizations / SCP conditions prevented faithful simulation.
    2. Returns verdict = None, [] (unverifiable status) rather than False (denied).
  • resolve_and_validate_role(...) treats verdict = None as a soft check: it warns the caller and proceeds with the role without raising RoleValidationError.
  • verify_hyperpod_connect_permissions(...) also handles unverifiable decisions gracefully by logging an INFO message and returning None.

3. Client-Side Validation Opt-Out

  • Added validate_role: bool = True optional parameter to resolve_and_validate_role(...). When set to False, client-side simulation and trust checks are skipped immediately.
  • Added environment variable control SAGEMAKER_VALIDATE_ROLE: setting SAGEMAKER_VALIDATE_ROLE=false (or 0 / no) skips client-side validation globally without requiring code modifications.

4. Object-Oriented Interface Integration

  • Exposed resolve_and_validate_role(...) as a method on the IamRoleResolver class so that call sites passing around IamRoleResolver instances can invoke validation directly while supporting the role_arn keyword alias.

Changes by File

File Changes
sagemaker-core/src/sagemaker/core/helper/iam_role_resolver.py • Added _is_unverifiable_denial helper.
• Refactored _simulate_denied_actions to return (denied, unverifiable) tuple.
• Updated _evaluate_permissions and verify_hyperpod_connect_permissions to log warnings and proceed on unverifiable verdicts.
• Added validate_role parameter and SAGEMAKER_VALIDATE_ROLE env var check to resolve_and_validate_role.
• Added resolve_and_validate_role method to IamRoleResolver class.
sagemaker-core/tests/unit/helper/test_iam_role_resolver.py • Updated existing TestSimulateDeniedActions unit tests for tuple return signature.
• Added test_partitions_unverifiable_denials covering conditional SCP and boundary evaluation results.
• Added test_conditional_scp_unverifiable_denial_warns_and_proceeds verifying RoleValidationError is bypassed with correct warning log.
• Added unit tests for validate_role=False, SAGEMAKER_VALIDATE_ROLE=false, IamRoleResolver().resolve_and_validate_role(), and HyperPod connect unverifiable checks.

Verification & Testing

Automated Unit Tests

Ran the full suite of unit tests for the helper module via pytest:

pytest sagemaker-core/tests/unit/helper

Results:

  • test_iam_role_resolver.py: 79 passed (100%)
  • Total helper suite (test_iam_role_creator.py, test_iam_role_resolver.py, test_session_helper.py): 204 passed in 2.14s

Backward Compatibility

  • Fully backward compatible: existing resolve_and_validate_role(...) callers and unit tests continue to function without modification.
  • Explicit and definitive IAM policy denials (explicitDeny or implicitDeny due to missing IAM role policy permissions) continue to raise RoleValidationError as expected.

Related Issues

…-based SCPs (aws#6019)

- Treat AllowedByOrganizations/AllowedByPermissionsBoundary=false with implicitDeny as unverifiable instead of definitive denial (_is_unverifiable_denial)

- Warn and proceed when policies cannot be definitively evaluated (�erdict = None)

- Add �alidate_role parameter to 
esolve_and_validate_role() and support SAGEMAKER_VALIDATE_ROLE=false env var to allow client-side check opt-out

- Expose 
esolve_and_validate_role class method on IamRoleResolver with 
ole_arn keyword alias support

- Add unit tests for conditional SCP handling, opt-out behavior, and class method
Copilot AI balanced review requested due to automatic review settings October 7, 2026 21:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

The merge of master into this branch (b4447a3) mis-resolved the
_simulate_denied_actions() conflicts, leaving the package unimportable:

- A stray "w" was appended to a string literal in
  verify_hyperpod_connect_permissions(), a SyntaxError that broke
  `import sagemaker.core` (helper/__init__ imports this module).
- verify_evaluation_caller_permissions(), added on master after this
  branch was cut, kept the pre-merge single-value call. It now bound the
  (denied, unverifiable) tuple to `denied`, so `if denied:` was always
  true and `", ".join(denied)` raised TypeError on every call.
- Two TestSimulateDeniedActions cases kept the single-value call while
  still asserting on `unverifiable`, raising NameError.

Also extend this fix to the new evaluation path: an implicitDeny caused
by a condition-based SCP or permissions boundary is unverifiable, not a
denial, so return None instead of raising RoleValidationError — the same
contract the role-validation and HyperPod paths already follow.

Adds unit tests for verify_evaluation_caller_permissions(), which had
none, covering the allowed, denied, unverifiable, non-role caller, and
cannot-simulate outcomes.

This branch is waiting to be deployed

1 waiting deployment
manual-approval — 9d9279d7 Waiting Oct 7, 2026 by mohamedzeidan2021 via wait-for-approval #1656
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants