[5/8] coordinator: support insecure manifests behind opt-in#2356
Conversation
msanft
force-pushed
the
split/pr-2337-runtime-nodeinstaller
branch
from
4a00b00 to
657abac
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
from
552c173 to
27802b6
Compare
msanft
changed the title
msanft
force-pushed
the
split/pr-2337-coordinator
branch
from
27802b6 to
7073c58
Compare
msanft
force-pushed
the
split/pr-2337-runtime-nodeinstaller
branch
from
657abac to
41b8d92
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
from
7073c58 to
af50bbe
Compare
msanft
force-pushed
the
split/pr-2337-runtime-nodeinstaller
branch
from
41b8d92 to
c596cca
Compare
msanft
force-pushed
the
split/pr-2337-initdata
branch
from
72898ef to
f2897e6
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
from
af50bbe to
65b519a
Compare
msanft
force-pushed
the
split/pr-2337-initdata
branch
from
f2897e6 to
91b1df7
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
from
65b519a to
891d7f0
Compare
msanft
force-pushed
the
split/pr-2337-initdata
branch
from
91b1df7 to
db7cd22
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
2 times, most recently
from
6223484 to
acaa9fd
Compare
msanft
force-pushed
the
split/pr-2337-initdata
branch
2 times, most recently
from
a8f6eb6 to
fd0d094
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
2 times, most recently
from
9eeeeba to
55e4947
Compare
msanft
force-pushed
the
split/pr-2337-initdata
branch
2 times, most recently
from
bbf67f4 to
9da6471
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
2 times, most recently
from
21d6bec to
1cd7565
Compare
msanft
force-pushed
the
split/pr-2337-initdata
branch
from
9da6471 to
aa5ba51
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
from
1cd7565 to
c26922a
Compare
msanft
force-pushed
the
split/pr-2337-initdata
branch
from
4e3d618 to
c168265
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
from
c26922a to
708ea30
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
from
708ea30 to
b1dd9dc
Compare
msanft
force-pushed
the
split/pr-2337-initdata
branch
from
c168265 to
28e1b1e
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
from
b1dd9dc to
746bc55
Compare
msanft
force-pushed
the
split/pr-2337-initdata
branch
from
28e1b1e to
464e044
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
from
746bc55 to
9b0cc73
Compare
msanft
force-pushed
the
split/pr-2337-initdata
branch
from
464e044 to
0762a6e
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
from
9b0cc73 to
50a85ba
Compare
msanft
force-pushed
the
split/pr-2337-initdata
branch
from
0762a6e to
1cff241
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
from
50a85ba to
abb75b8
Compare
msanft
force-pushed
the
split/pr-2337-initdata
branch
from
1cff241 to
62ac3e2
Compare
msanft
force-pushed
the
split/pr-2337-initdata
branch
from
62ac3e2 to
9d6a048
Compare
msanft
force-pushed
the
split/pr-2337-coordinator
branch
from
abb75b8 to
2701421
Compare
charludo
left a comment
charludo
left a comment
There was a problem hiding this comment.
Thanks! Sorry review took so long. I have a couple of comments, see below; the one architectural one I want to get your opinion on is if we should allow mixed manifests. Is that something you actually require? Because if not, then I'd strongly urge to allow either all-secure, or all-insecure manifests, no mix.
| if !g.allowInsecure && mnfst.AllowInsecure() { | ||
| return nil, ErrInsecureNotAllowed | ||
| } |
There was a problem hiding this comment.
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?
| // | ||
| // 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 { |
There was a problem hiding this comment.
Not super happy with just passing an unnamed bool into here. Invocations don't make it super obvious what a serious effect this has. How would you feel about instead leaving New as-is, without the option to allowInsecure, and instead add something like func (g *Guard) MakeInsecure() that needs to be explicitly called?
| default: | ||
| return nil, fmt.Errorf("unsupported platform: %T", cpuid.CPU) | ||
| log.Warn("No TEE platform detected, using insecure attestation issuer") | ||
| return insecure.NewIssuer(), nil | ||
| } |
There was a problem hiding this comment.
This should not default to insecure. Any chance to something similar here as with allowInsecureAttestation() in the initedata processor?
| logger.NewWithAttrs(logger.NewNamed(c.logger, "validator"), map[string]string{"reference-values": name}), &authInfo, name)) | ||
| } | ||
|
|
||
| if state.Manifest().AllowInsecure() { |
There was a problem hiding this comment.
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 in
if !g.allowInsecure && mnfst.AllowInsecure() {
return nil, ErrInsecureNotAllowed
}below, that is true when any platform allows insecure.
| _, allowInsecure := os.LookupEnv(allowInsecureEnvVar) | ||
| if allowInsecure { | ||
| logger.Warn("Coordinator is configured to allow insecure manifests") | ||
| } |
There was a problem hiding this comment.
Check value, not just presence. CONTRAST_ALLOW_INSECURE=0 should not enable insecure mode. I can easily see users deliberately "disabling" insecure mode like this. strconv.ParseBool should do the job.
2 participants
Split from #2337 as part of a stacked review series.
Depends on: #2355
This PR wires insecure manifests into the Coordinator behind explicit opt-in: