Skip to content

atls: move validators into dedicated package #2483

Open
burgerdev wants to merge 3 commits into
mainfrom
burgerdev/validators-pkg
Open

atls: move validators into dedicated package #2483
burgerdev wants to merge 3 commits into
mainfrom
burgerdev/validators-pkg

Conversation

@burgerdev

@burgerdev burgerdev commented Jun 26, 2026

Copy link
Copy Markdown
Member

My overall plan is to consolidate the attestation validation logic into one package. We currently construct validators (at least) here:

contrast/sdk/common.go

Lines 27 to 29 in 41ae00c

func ValidatorsFromManifest(kdsGetter *certcache.CachedHTTPSGetter, m *manifest.Manifest, log *slog.Logger) ([]atls.Validator, error) {
var validators []atls.Validator

var validators []atls.Validator

opts, err := manifestParsed.SNPValidateOpts(kdsGetter)
require.NoError(err, "getting SNP validate options")
var validators []atls.Validator
for i, opt := range opts {

Since the manifest is the authoritative source for admission policy, I would like to move all of this into the manifest package.

Such a refactoring is currently hard to do, because what were supposed to be validation internals leak throughout the code base.

  • The reportSetter is a special purpose implementation for aTLS only, but leaves its traces in validator constructors and duplicated implementations. If validators returned a report struct it would not be necessary, as the Coordinator could just wrap the validator.
  • The OID Getter design only makes sense in this particular loop over validators:
    foundExtension = true
    for _, validator := range validators {
    // Optimization: Skip the validator if it doesn't match the attestation type of the document.
    if !ex.Id.Equal(validator.OID()) {
    continue
    }
    Now we're forced to present an exact list of validators that can each only validate one type of document. But the Coordinator can't wrap a list.
  • Something that's been bothering me for quite some time: an empty list of validators means "skip validation", while a populated list means "at least one validator must pass". The logic should not surprise people assuming vacuous truth . I want my sums of empty lists to be 0.
    // don't perform verification of attestation document if no validators are set
    if len(c.validators) == 0 {
    return nil
    }
    return verifyEmbeddedReport(ctx, c.validators, cert, pubBytes, c.clientNonce)
    If we did not deal in lists, this would be trivial to fix in a validators package.

That being said, this PR takes an initial step into this direction.

  • Clean up around the existing validator definition (first two commits).
  • Move the Validator into its own package.

No functional change, but this paves the way for above plan.

The only user of this struct does not call any of its methods. Add a
local fake in the recovery_test and remove the fake from atls.
It's only used by internal tests.
This sets the stage for adding validator helper methods.
@burgerdev burgerdev added the no changelog PRs not listed in the release notes label Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changelog PRs not listed in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant