Add validations on EC2 enclave pre-init #1159
Conversation
scripts/aws/ec2.py
Outdated
|
|
||
| def __init__(self): | ||
| super().__init__() | ||
| self.config = {} |
There was a problem hiding this comment.
Below the member variable being used appears to be self.configs.
scripts/aws/ec2.py
Outdated
| token_response = requests.put(token_url, headers={"X-aws-ec2-metadata-token-ttl-seconds": "3600"}, timeout=2) | ||
| return token_response.text | ||
| except Exception as e: | ||
| return "blank" |
There was a problem hiding this comment.
Why not let the exception propagate through? Why is it valid to swallow the error here?
| raise Exception("Unable to access secret store") | ||
|
|
||
| def __add_defaults(self, configs): | ||
| configs.setdefault("enclave_memory_mb", 24576) |
There was a problem hiding this comment.
Where are these default coming from? Why are they reasonable?
scripts/aws/ec2.py
Outdated
| def cleanup(self): | ||
| describe_output = subprocess.check_output(["nitro-cli", "describe-enclaves"], text=True) | ||
| enclaves = json.loads(describe_output) | ||
| enclave_id = enclaves[0].get("EnclaveID") if enclaves else None |
There was a problem hiding this comment.
This just kills the first enclave from the list without consideration to whether it is the right one.
There was a problem hiding this comment.
we only run 1 enclave on a host, right?
can add validation if needed
scripts/aws/ec2.py
Outdated
| ec2 = EC2() | ||
| if args.operation and args.operation == "stop": | ||
| ec2.cleanup() | ||
| [ec2.kill_process(process) for process in ["vsockpx", "sockd", "vsock-proxy", "nohup"]] |
There was a problem hiding this comment.
Why is nohup in this list? This could potentially kill completely unrelated processes. And why are there both vsockpx and vsock-proxy in the list?
There was a problem hiding this comment.
| Validates operator key if following new pattern. Ignores otherwise | ||
| """ | ||
| api_token = secrets.get('api_token', None) | ||
| pattern = r"^(UID2|EUID)-.\-(I|P)-\d+-\*$" |
There was a problem hiding this comment.
Are you sure all operator keys follow this pattern?
There was a problem hiding this comment.
If it doesn't; we ignore since it is an old key
scripts/confidential_compute.py
Outdated
| api_token = secrets.get('api_token', None) | ||
| pattern = r"^(UID2|EUID)-.\-(I|P)-\d+-\*$" | ||
| if bool(re.match(pattern, api_token)): | ||
| if secrets.get('debug_mode', False) or secrets.get('environment') == 'integ': |
There was a problem hiding this comment.
Why are operator keys forced to integ in debug mode? If debug mode is allowed only for integ, there should be an explicit check for that.
There was a problem hiding this comment.
Ok. Can update.
scripts/confidential_compute.py
Outdated
| Validates core/optout is accessible. | ||
| """ | ||
| try: | ||
| core_ip = socket.gethostbyname(urlparse(config['core_base_url']).netloc) |
There was a problem hiding this comment.
to provide a meaningful error .
f"Failed to reach required URLs. Consider enabling {core_ip}, {optout_ip} in the egress firewall."
|
|
||
| except (requests.ConnectionError, requests.Timeout) as e: | ||
| raise Exception( | ||
| f"Failed to reach required URLs. Consider enabling {core_ip}, {optout_ip} in the egress firewall." |
There was a problem hiding this comment.
is it worth distinguishing here between DNS resolution failure and the actual request?
There was a problem hiding this comment.
Egress rules are written based on IP and not DNS. If a company implements strict egress rule (for security) they usually will need these IP's to be enabled.
| @@ -80,6 +80,7 @@ else | |||
| exit 1 | |||
| fi | |||
|
|
|||
| # DO WE NEED THIS? do we expect customers to change URL? | |||
There was a problem hiding this comment.
I've never heard of it being done before.
| def cleanup(self) -> None: | ||
| """Terminates the Nitro Enclave and auxiliary processes.""" | ||
| try: | ||
| describe_output = subprocess.check_output(["nitro-cli", "describe-enclaves"], text=True) |
There was a problem hiding this comment.
why not just nitro-cli terminate-enclave --all
scripts/confidential_compute.py
Outdated
| env = secrets.get("environment", "").lower() | ||
| debug_mode = secrets.get("debug_mode", False) | ||
| expected_env = "I" if debug_mode or env == "integ" else "P" | ||
| if api_token.split("-")[2] != expected_env: |
There was a problem hiding this comment.
also throw error if debug_mode and env == prod? Before attestation failure happens.
There was a problem hiding this comment.
Yep, that can be added as another check 👍
| self.__run_config_server(log_level) | ||
| self.__run_socks_proxy(log_level) | ||
|
|
||
| def _validate_auxiliaries(self) -> None: |
There was a problem hiding this comment.
auxiliaries is meaningless - just say Config Server?
for every function that does validation, add some comments to say what you are testing as std out description.
UID2- 4364
Looking for feedback on approach; migrating to python from bash to improve validations, readability and error handling.
Follow up tickets will extend this to all Cloud providers, and add custom exceptions that links to public documentation.