gitrepo: add support for Git tag verification

[aryan9600]

Add three new verification modes for .spec.verify.mode:

• HEAD: Verify the commit that the HEAD of the repo points to after checking out to the ref specified in .spec.ref. Its the same as head, which cannot be removed due to backwards compatibility reasons and is converted to HEAD internally.
• Tag: Verify the tag referred to by .spec.ref.tag.
• TagAndHEAD: Verify the tag referred to by .spec.ref.tag and the commit that the tag points to.

The default is HEAD, to ensure backwards compatibility.

Furthermore, add .status.sourceVerificationMode to record the last successful verification mode used. This is then used to determine whether we need to (re)verify any Git objects depending on the latest verification configuration.

Fixes: #1133

[darkowlzz]

GitRepository spec docs also need to be updated for this change.

[hiddeco]

This looks really good, thanks for following up with the review comments 🥇 💯

[darkowlzz]

I encountered this situation when I changed the verification mode from TagAndHEAD to Tag or anything else:

spec:
  interval: 1h
  ref:
    tag: 6.4.1
  timeout: 60s
  url: https://github.com/stefanprodan/podinfo
  verify:
    mode: Tag
    secretRef:
      name: pgp-public-keys
status:
  artifact:
    ...
  conditions:
  - lastTransitionTime: "2023-08-14T14:11:35Z"
    message: stored artifact for revision '6.4.1@sha1:4892983fd12e3ffffcd5a189b1549f2ef26b81c2'
    observedGeneration: 18
    reason: Succeeded
    status: "True"
    type: Ready
  - lastTransitionTime: "2023-08-14T13:38:34Z"
    message: stored artifact for revision '6.4.1@sha1:4892983fd12e3ffffcd5a189b1549f2ef26b81c2'
    observedGeneration: 18
    reason: Succeeded
    status: "True"
    type: ArtifactInStorage
  - lastTransitionTime: "2023-08-14T14:17:51Z"
    message: verified signature of commit '4892983fd12e3ffffcd5a189b1549f2ef26b81c2'
      with key '4AEE18F83AFDEB23' and signature of tag '6.4.1@0edeca4929922cba44c87dde9e20b2c6c3cc53ab'
      with key '3299AEB0E4085BAF'
    observedGeneration: 15
    reason: VerifiedTagAndCommit
    status: "True"
    type: SourceVerified
  observedGeneration: 18
  observedVerificationMode: TagAndHEAD

The observations in the status don't reflect the current spec when no re-verification is required.

Also, if the verification is removed, the status contains stale data:

spec:
  interval: 1h
  ref:
    tag: 6.4.1
  timeout: 60s
  url: https://github.com/stefanprodan/podinfo
status:
  artifact:
    ...
  conditions:
  - lastTransitionTime: "2023-08-14T14:11:35Z"
    message: stored artifact for revision '6.4.1@sha1:4892983fd12e3ffffcd5a189b1549f2ef26b81c2'
    observedGeneration: 20
    reason: Succeeded
    status: "True"
    type: Ready
  - lastTransitionTime: "2023-08-14T13:38:34Z"
    message: stored artifact for revision '6.4.1@sha1:4892983fd12e3ffffcd5a189b1549f2ef26b81c2'
    observedGeneration: 20
    reason: Succeeded
    status: "True"
    type: ArtifactInStorage
  - lastTransitionTime: "2023-08-14T14:17:51Z"
    message: verified signature of commit '4892983fd12e3ffffcd5a189b1549f2ef26b81c2'
      with key '4AEE18F83AFDEB23' and signature of tag '6.4.1@0edeca4929922cba44c87dde9e20b2c6c3cc53ab'
      with key '3299AEB0E4085BAF'
    observedGeneration: 15
    reason: VerifiedTagAndCommit
    status: "True"
    type: SourceVerified
  observedGeneration: 20
  observedVerificationMode: TagAndHEAD

Is that okay? or maybe we should update the status without doing a re-verification?

[hiddeco]

Given the above, I am now in addition wondering if a message constructed more like something in this format wouldn't be much easier to read:

verified signature of tag and commit:
- '6.4.1@sha1:0edeca4929922cba44c87dde9e20b2c6c3cc53ab' using key '3299AEB0E4085BAF'
- 'sha1:4892983fd12e3ffffcd5a189b1549f2ef26b81c2' using key '4AEE18F83AFDEB23'

[aryan9600]

thats by design, the status only gets updated when we re-verify something different. in both scenarios, the information displayed by .status is not technically false since it did verify both the tag and HEAD even if the current configurations asks for just tag verification. or if the verification config is removed, the status still has information about verification since it did verify the object (i wouldn't call it stale data).

[darkowlzz]

NOTE: Old observation based on misunderstanding, please read the next comment.

Tried a few more things and the status seemed to became more unhelpful due to the stale observed verification mode. For example:

spec:
  interval: 1h
  ref:
    branch: master
  timeout: 60s
  url: https://github.com/stefanprodan/podinfo
  verify:
    mode: Tag
    secretRef:
      name: pgp-public-keys
status:
  artifact:
    ...
  conditions:
  - lastTransitionTime: "2023-08-14T20:11:09Z"
    message: cannot verify tag object's signature if a tag reference is not specified
    observedGeneration: 5
    reason: InvalidVerificationMode
    status: "True"
    type: Stalled
  - lastTransitionTime: "2023-08-14T20:09:17Z"
    message: cannot verify tag object's signature if a tag reference is not specified
    observedGeneration: 5
    reason: InvalidVerificationMode
    status: "False"
    type: Ready
  - lastTransitionTime: "2023-08-14T20:09:17Z"
    message: cannot verify tag object's signature if a tag reference is not specified
    observedGeneration: 5
    reason: InvalidVerificationMode
    status: "False"
    type: SourceVerified
  - lastTransitionTime: "2023-08-14T20:05:36Z"
    message: stored artifact for revision '6.4.1@sha1:4892983fd12e3ffffcd5a189b1549f2ef26b81c2'
    observedGeneration: 2
    reason: Succeeded
    status: "True"
    type: ArtifactInStorage
  observedGeneration: 5
  observedVerificationMode: TagAndHEAD

Spec has verification mode as Tag, observed verification mode is TagAndHEAD. Usually we reason about things based on the status but in this case, status seems to be making it more confusing, as if the reconciler didn't see what was specified in the spec.

I tried two things which helped me.

• In verifySignature(), at the beginning of the function setting the observed verification mode, before verifying, as this observation is independent of the result of verification

obj.Status.ObservedVerificationMode = ptrToVerificationMode(obj.Spec.Verification.GetMode())

• In reconcileSource(), after if !gitContentConfigChanged(obj, includes) {, if it's a no-op reconciliation, we do update the ArtifactInStorage condition to make sure that the status condition is up to date and also remove any stale FetchFailed condition. I updated the observations there:

// Update observations.
if obj.Status.ObservedVerificationMode != nil && obj.Spec.Verification == nil {
	obj.Status.ObservedVerificationMode = nil
}
if obj.Spec.Verification != nil {
	obj.Status.ObservedVerificationMode = ptrToVerificationMode(obj.Spec.Verification.GetMode())
}

These two changes made the status reasonable for the above situation and for the situation when the mode is changed from TagAndHEAD to anything else

spec:
  interval: 1h
  ref:
    branch: master
  timeout: 60s
  url: https://github.com/stefanprodan/podinfo
  verify:
    mode: Tag
    secretRef:
      name: pgp-public-keys
status:
  artifact:
    ...
  conditions:
  - lastTransitionTime: "2023-08-14T20:42:38Z"
    message: cannot verify tag object's signature if a tag reference is not specified
    observedGeneration: 5
    reason: InvalidVerificationMode
    status: "True"
    type: Stalled
  - lastTransitionTime: "2023-08-14T20:42:38Z"
    message: cannot verify tag object's signature if a tag reference is not specified
    observedGeneration: 5
    reason: InvalidVerificationMode
    status: "False"
    type: Ready
  - lastTransitionTime: "2023-08-14T20:42:38Z"
    message: cannot verify tag object's signature if a tag reference is not specified
    observedGeneration: 5
    reason: InvalidVerificationMode
    status: "False"
    type: SourceVerified
  - lastTransitionTime: "2023-08-14T20:30:37Z"
    message: stored artifact for revision '6.4.1@sha1:4892983fd12e3ffffcd5a189b1549f2ef26b81c2'
    observedGeneration: 4
    reason: Succeeded
    status: "True"
    type: ArtifactInStorage
  observedGeneration: 5
  observedVerificationMode: Tag

[darkowlzz]

Another thought, I think there's also a confusion in what observedVerificationMode means by its name. The other observed* status fields we have are used to record the previous configuration and detect a change in the source configuration. But in this case, I think the field was added with the intention of storing what was verified, and not what was observed in the spec, hence the code sets this observation only after verifying. I think this should be called something else, without observed* prefix as it's the result of something and not an observation. I think that'll make the status less confusing.
Maybe sourceVerificationMode, artifactVerificationMode or something that doesn't mix this with the other observed* fields in the status and indicates that it's about the artifact in the storage and not about the current reconciliation or version of the object, that may be failing.
Related to this, the new reason values for SourceVerifiedCondition seems to be redundant information as this new field expresses the same. Presence of verification mode in the status means that verification was successful for the artifact in storage and also tells how it was verified. An observer is equally likely to see the conditions and this field, no difference for metrics collectors or any other reader, except if we want to expose this detail using the conditions.Getter interface.

Update:
Reading the spec docs Observed Verification Mode section

The observed verification mode is the latest .spec.observedVerificationMode .spec.verify.mode value which resulted in a ready state, or stalled due to error it can not recover from without human intervention. The value is the same as the verification mode in spec. It indicates the verification configuration used in verifying Git objects.

This appears to match with my initial understanding of the field but I think it should be changed to say that it's less about the state of the object, ready/stalled/failing, and more about the artifact in storage, as a change in git reference can result in failing reconciliation but the SourceVerifiedCondition will remain to be true in the status for the old artifact with the condition observed generation of when the artifact was built successfully.

[darkowlzz]

We had a discussion about it in a meeting today and there was agreement on a few things:

• To make it more clear, rename observedVerificationMode to sourceVerificationMode, in line with other fields like IncludedArtifacts that's more linked to the result of reconciliation, to make it more explicit that it's not about what's in the spec and more about the artifact being served. There were more considerations about the atomicity of how this field is set in reconcileSource() before the artifact is actually made in reconcileArtifact(), but since we don't have such atomicity in the current design, just renaming should be good enough for now.
• Remove the new reasons to remain consistent with other APIs like OCIRepository which also have SourceVerifiedCondition condition. A reader that just want to check if verification was performed can read SourceVerifiedCondition in a generic way for any kind of source. If more details are needed, they will have to check the respective field of their source kinds, .status.sourceVerificationMode in this case and maybe some other similar field for OCIRepository.

We can still discuss more if needed.

[aryan9600]

Spec has verification mode as Tag, observed verification mode is TagAndHEAD. Usually we reason about things based on the status but in this case, status seems to be making it more confusing, as if the reconciler didn't see what was specified in the spec.

your next comment regarding the meaning of .status.observedVerificationMode resolves this issue implicitly. if we change it to .status.sourceVerificationMode (or something that indicates the last mode used for verification), then its fine for it to be different than .spec.verify.mode.

Remove the new reasons to remain consistent with other APIs like OCIRepository which also have SourceVerifiedCondition condition.

yes, now that we are recording the verification mode used in the status of the object, there's no need for a more explicit reason.

[darkowlzz]

It may be irrelevant and could be ignored, but I just had a thought that these new constants Mode* don't have any prefix to indicate that they are about git source, being in a common v1 package. Looking at the other constants and types in the api package, it looks like most of the things that are specific to a source kind have something in the name to indicate that.
It may be nothing, but it just came to my mind.

[aryan9600]

didn't add one because its not just a plain string but rather a typed string, so the type indicates the fact that these constants are related to Git. but, if later we want to add something like ModeTag OCIVerificationMode, we won't be able to, so yes lets change it to something like ModeGitTag?

[darkowlzz]

Sounds better. I'm hesitant to suggest GitVerificationModeHEAD, GitVerificationModeTag and GitVerificationModeTagAndHEAD in case there's a need to keep it short, but I personally would go with the long version. Maybe get some more opinions on it as it's in v1 package and we won't be able to get rid of it soon/easily.

[aryan9600]

i do indeed think its too long 😬

[darkowlzz]

Did some manual testing and overall, it LGTM!