Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure verify terminates on model validation failure #352

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

miyunari
Copy link
Contributor

@miyunari miyunari commented Jan 27, 2025

Summary

Ensure verify terminates on model validation failure.

Closes #351

Release Note

fix: ensure verify terminates on model validation failure

Documentation


Now it seems to work as expected :)

Init Containers:
  model-validation:
    Container ID:  containerd://6b4dfaa5cb32191638bb57fa77a7dc1ff0f4541ea65ebac047556e0bea636bad
    Image:         ghcr.io/miyunari/model-transparency-cli:latest
    Image ID:      ghcr.io/miyunari/model-transparency-cli@sha256:57e80c133028dbc28d1be265756dc8e2382a0891b6c6a4662854623f70c30ff2
    Command:
      verify
      --model_path=/data
      --sig_path=/data/model.sig
      sigstore
      --identity
      https://github.com/miyunari/model-validation-controller/.github/workflows/sign-model.yaml@refs/tags/v0.0.2
      --identity-provider
      https://token.actions.githubusercontent.com
    State:          Terminated           <<-------------------------------
      Reason:       Error
      Exit Code:    255
      Started:      Mon, 27 Jan 2025 11:46:59 +0100
      Finished:     Mon, 27 Jan 2025 11:47:00 +0100

@miyunari miyunari requested review from a team as code owners January 27, 2025 10:35
@miyunari miyunari force-pushed the fix/cli_verification branch from 02e86cf to b914c4d Compare January 27, 2025 10:46
@miyunari miyunari force-pushed the fix/cli_verification branch from b914c4d to cc3f10f Compare January 27, 2025 10:50
@mihaimaruseac
Copy link
Collaborator

I think we should not use os._exit. Documentations says to only use in child process after a fork call.

Also, the exit constant used elsewhere in the script is actually raising an exception (per documentation), which is also not something we likely want.

We should most likely use sys.exit everywhere.

Consider this test program:

import atexit
import os
import sys

class C:
  def __del__(self):
    print("Destructor called")

def test_exit():
  exit(-1)

def test_sys_exit():
  sys.exit(-1)

def test_os__exit():
  os._exit(-1)

def bye():
  print("bye...")

c = C()
atexit.register(bye)

args = sys.argv[1:]
if len(args) == 1:
  what = args[0]
  if what == "exit": test_exit()
  if what == "sys.exit": test_sys_exit()
  if what == "os._exit": test_os__exit()
  print("Nothing matched")

print("Ended successfully")

Running it with python -S:

[...]$ python -S test.py exit; echo $?
Traceback (most recent call last):
  File "/tmp/test.py", line 27, in <module>
    if what == "exit": test_exit()
                       ^^^^^^^^^^^
  File "/tmp/test.py", line 10, in test_exit
    exit(-1)
    ^^^^
NameError: name 'exit' is not defined. Did you mean: 'atexit'?
bye...
Destructor called
1
[...]$ python -S test.py sys.exit; echo $?
bye...
Destructor called
255
[...]$ python -S test.py os._exit; echo $?
255

Only sys.exit performed GC, called the destructor, called the atexit handler, did proper cleanup. The exit builtin was disabled by the -S switch, it is only present to help interpreters.

It seems at the selected version (1fa9614) there is no exit present. That got fixed in #348. Do you mind updating the PR to use sys.exit everywhere, instead of exit?

@spencerschrock
Copy link
Contributor

[...]$ python -S test.py os._exit; echo $?
255

Unrelated, but should this be sys.exit(1) then? -1 is out of range for exit codes which is why we get a 255.

Do you mind updating the PR to use sys.exit everywhere

If so, I'd like to request all the -1 exit codes to be changed to 1.

@mihaimaruseac
Copy link
Collaborator

Oh, right, that's a good point. -1 is from kernel to userspace, otherwise it should be positive. Thanks for catching it

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.

Invalid signature does not lead to failed verification
3 participants