Skip to content

Add local file-based access-control rule support.#329

Open
ZhidongPeng wants to merge 4 commits intoAzure:devfrom
ZhidongPeng:dev
Open

Add local file-based access-control rule support.#329
ZhidongPeng wants to merge 4 commits intoAzure:devfrom
ZhidongPeng:dev

Conversation

@ZhidongPeng
Copy link
Copy Markdown
Collaborator

  • Added base64 = "0.22" dependency
  • Introduces a new local_rules module that

parses base64-encoded rule-id descriptors,
merges host-delivered and customer-managed rules with fail-closed behavior on parse errors,
tracks file state across polls, and integrates with key_keeper

  • Added rules_dir: PathBuf field to KeyKeeper struct and refactored update_access_control_rules() to accept state tracker, call resolve_effective_rules() for WireServer/IMDS/HostGA, and handle local-rule-merged effective rules

Copy link
Copy Markdown
Collaborator Author

@ZhidongPeng ZhidongPeng left a comment

Choose a reason for hiding this comment

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

PR Review ??? Add local file-based access-control rule support

Thanks for the PR. Overall the design is clean: base64-encoded rule-id descriptors, fail-closed on parse errors, and file-state tracking across polls are all solid patterns.

Below are inline comments ranging from a potential behavioral regression to minor nits. Please take a look.

let local_rules_file = rules_dir.join(target.file_name());
let current_file_state = get_local_rule_file_state(&local_rules_file);
let file_state_changed = tracker.file_state != current_file_state;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Minor] There is a small TOCTOU window between checking get_local_rule_file_state (metadata/mtime) and actually reading the file later (line 351). The file could be modified or deleted in between. The retry logic in read_local_rules_file partly mitigates this, but in the worst case:

  1. get_local_rule_file_state sees Present(T1)
  2. File is deleted
  3. read_local_rules_file fails all retries ??? fail-closed (correct behavior)

So the retry + fail-closed combination makes this safe in practice. Just wanted to flag it for awareness.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant