-
Notifications
You must be signed in to change notification settings - Fork 19
[5/8] coordinator: support insecure manifests behind opt-in #2356
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
base: split/pr-2337-initdata
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,10 @@ var ( | |
| // ErrConcurrentUpdate is returned by state-modifying operations if the input oldState is not | ||
| // the current state. This usually happens when a concurrent operation succeeded. | ||
| ErrConcurrentUpdate = errors.New("coordinator state was updated concurrently") | ||
|
|
||
| // ErrInsecureNotAllowed is returned when a manifest contains insecure platforms but the | ||
| // coordinator was not started with the allow-insecure flag. | ||
| ErrInsecureNotAllowed = errors.New("manifest contains insecure platforms, but the coordinator is not configured to allow them") | ||
| ) | ||
|
|
||
| // Guard manages the manifest state of Contrast. | ||
|
|
@@ -65,6 +69,9 @@ type Guard struct { | |
| logger *slog.Logger | ||
| metrics metrics | ||
|
|
||
| // allowInsecure controls whether manifests with insecure platforms are accepted. | ||
| allowInsecure bool | ||
|
|
||
| clock clock.Clock | ||
| } | ||
|
|
||
|
|
@@ -73,7 +80,10 @@ type metrics struct { | |
| } | ||
|
|
||
| // New creates a new state Guard instance. | ||
| func New(hist *history.History, reg *prometheus.Registry, log *slog.Logger) *Guard { | ||
| // | ||
| // If allowInsecure is true, the Guard will accept manifests that contain insecure platforms. | ||
| // Otherwise, setting such a manifest will be rejected with ErrInsecureNotAllowed. | ||
| func New(hist *history.History, reg *prometheus.Registry, log *slog.Logger, allowInsecure bool) *Guard { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not super happy with just passing an unnamed |
||
| manifestGeneration := promauto.With(reg).NewGauge(prometheus.GaugeOpts{ | ||
| Subsystem: "contrast_coordinator", | ||
| Name: "manifest_generation", | ||
|
|
@@ -82,8 +92,9 @@ func New(hist *history.History, reg *prometheus.Registry, log *slog.Logger) *Gua | |
| manifestGeneration.Set(0) | ||
|
|
||
| return &Guard{ | ||
| hist: hist, | ||
| logger: log.WithGroup("stateguard"), | ||
| hist: hist, | ||
| logger: log.WithGroup("stateguard"), | ||
| allowInsecure: allowInsecure, | ||
| metrics: metrics{ | ||
| manifestGeneration: manifestGeneration, | ||
| }, | ||
|
|
@@ -271,6 +282,9 @@ func (g *Guard) UpdateState(_ context.Context, oldState *State, se *seedengine.S | |
| if err := json.Unmarshal(manifestBytes, &mnfst); err != nil { | ||
| return nil, fmt.Errorf("unmarshaling manifest: %w", err) | ||
| } | ||
| if !g.allowInsecure && mnfst.AllowInsecure() { | ||
| return nil, ErrInsecureNotAllowed | ||
| } | ||
|
Comment on lines
+285
to
+287
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry if this has been decided already, I might just be OOTL and wanted to make sure: Do we also care about the reverse? I.e. is it acceptable to allow users to have a secure deployment run with an insecure coordinator? |
||
| policyMap := make(map[[history.HashSize]byte][]byte) | ||
| for _, policy := range policies { | ||
| policyHash, err := g.hist.SetPolicy(policy) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,7 @@ import ( | |
|
|
||
| const ( | ||
| metricsEnvVar = "CONTRAST_METRICS" | ||
| allowInsecureEnvVar = "CONTRAST_ALLOW_INSECURE" | ||
| probeAndMetricsPort = 9102 | ||
| // transitEngineAPIPort specifies the default port to expose the transit engine API. | ||
| transitEngineAPIPort = "8200" | ||
|
|
@@ -115,7 +116,12 @@ func run() (retErr error) { | |
|
|
||
| hist := history.NewWithStore(logger.WithGroup("history"), store) | ||
|
|
||
| meshAuth := stateguard.New(hist, promRegistry, logger) | ||
| _, allowInsecure := os.LookupEnv(allowInsecureEnvVar) | ||
| if allowInsecure { | ||
| logger.Warn("Coordinator is configured to allow insecure manifests") | ||
| } | ||
|
Comment on lines
+119
to
+122
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check value, not just presence. |
||
|
|
||
| meshAuth := stateguard.New(hist, promRegistry, logger, allowInsecure) | ||
|
|
||
| issuer, err := issuer.New(logger) | ||
| if err != nil { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,10 +6,10 @@ | |
| package issuer | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "log/slog" | ||
|
|
||
| "github.com/edgelesssys/contrast/internal/atls" | ||
| "github.com/edgelesssys/contrast/internal/attestation/insecure" | ||
| snpissuer "github.com/edgelesssys/contrast/internal/attestation/snp/issuer" | ||
| tdxissuer "github.com/edgelesssys/contrast/internal/attestation/tdx/issuer" | ||
| "github.com/edgelesssys/contrast/internal/logger" | ||
|
|
@@ -29,6 +29,7 @@ func New(log *slog.Logger) (atls.Issuer, error) { | |
| logger.NewWithAttrs(logger.NewNamed(log, "issuer"), map[string]string{"tee-type": "tdx"}), | ||
| ), nil | ||
| default: | ||
| return nil, fmt.Errorf("unsupported platform: %T", cpuid.CPU) | ||
| log.Warn("No TEE platform detected, using insecure attestation issuer") | ||
| return insecure.NewIssuer(), nil | ||
| } | ||
|
Comment on lines
31
to
33
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should not default to insecure. Any chance to something similar here as with |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO there need to be two variants of
AllowInsecure. One to use here, that is only true when all platforms allow insecure in the manifest (just as an additional guard), and the other one in the current form for use inbelow, that is true when any platform allows insecure.