Skip to content
This repository was archived by the owner on Sep 3, 2025. It is now read-only.

Conversation

@csucu
Copy link
Contributor

@csucu csucu commented Nov 28, 2022

@csucu csucu requested review from dmotylev and jpicht November 28, 2022 10:05
Copy link
Contributor

@dmotylev dmotylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of suggestions regarding naming, which might add some clarity w/o much documenting

@csucu csucu force-pushed the feature/arc-support branch from 0c9c163 to c34228c Compare December 1, 2022 20:36
@csucu csucu marked this pull request as ready for review December 5, 2022 10:45
@csucu csucu changed the title wip - arc support arc verification Dec 5, 2022
@csucu csucu changed the title arc verification Add arc verification Dec 5, 2022
@csucu csucu changed the title Add arc verification Add ARC verification Dec 5, 2022
Copy link

@jpicht jpicht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor things, looks good otherwise.

arc.go Outdated
amsKey = "ARC-Message-signature"
aarKey = "ARC-Authentication-Results"

errMissingArcFields = errors.New("missing arc fields")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't errors be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
// if "c=" not given, use relaxed
if !ams.canonicalization {
ams.RelaxedHeader = true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this always be true already?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On as, yes. The ams is the result of parseSignature(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea on AS, on the AMS header sometimes the cannonization field is not provided, so we just want a default here.

arc_test.go Outdated

for testName, test := range doc.Tests {
t.Run(fmt.Sprintf("%s", testName), func(t *testing.T) {
//if testName != "ams_fields_c_sr" {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove dead code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

dkim.go Outdated
// Only used for arc, so only contains the used result codes
// https://www.rfc-editor.org/rfc/rfc8617.html#section-4.4
func extractResultCode(value string) ResultCode {
//todo: check if we need this bit
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was an old comment, removed

@csucu csucu force-pushed the feature/arc-support branch 2 times, most recently from c223a0f to aa8f3a7 Compare December 7, 2022 14:43
@csucu csucu force-pushed the feature/arc-support branch from 470ed66 to c4cccf1 Compare December 7, 2022 15:54
@csucu csucu requested a review from dmotylev January 5, 2023 16:19
@csucu csucu requested a review from jpicht January 8, 2023 15:35
@csucu csucu merged commit 30f3bd8 into master Jan 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants