LGTM otherwise.

Codecov Report

Merging #89 into master will decrease coverage by 0.23%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
- Coverage   79.49%   79.26%   -0.24%     
==========================================
  Files          15       15              
  Lines         995     1003       +8     
==========================================
+ Hits          791      795       +4     
- Misses        128      132       +4     
  Partials       76       76              

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f828d02...ce2b371. Read the comment docs.

Question: should Header.Add return an error, or should we return an error when formatting the header?

• Returning an error in Header.Add makes it a little bit annoying for callers to add headers since they need to error-check each time. Also, existing users may have header fields silently dropped, so this may be confusing/annoying to debug.
• Returning an error when formatting the header may make it more difficult to trace which Header.Add call is responsible for the malformed header.

Returning an error when formatting the header may make it more difficult to trace which Header.Add call is responsible for the malformed header.

Adding enough information to the error message to identify exact field that caused it can help.

I agree with your concerns regarding Header.Add. It is unexpected that Header.Add can fail and some people just ignore errors if it gets too verbose and "cannot possibly fail here".

Therefore I think it is a good idea to move check to the formatting time and make error look like "Nth DKIM-Signature value contains invalid characters"

I moved checks to WriteHeader. However I'm not sure about error message format and the fact that we go there from the last header towards the first and error in header k does not mean any of k-n is fine.

Regarding rebase to master, in c512562 header type check was moved to a separate function, and I honestly have no idea how keep it clean and informative about what exact header is incorrect

Maybe this?

if _, err := w.Write(f.raw()); err != nil {
    return fmt.Errorf("failed to write header field #%v (%q): %w", len(h.l) - i, f.k, err)
}

But I wonder if it's a good idea to export DisallowedCharacterError at all. Maybe we should wait for someone to ask for it. Depending on the use-case it may make sense to have e.g. HeaderFieldError.

Maybe this?

In your example err is returned by writer.
There is the closest code I can come up with, in case we add this check to raw() or something close if there will be another validation function.

if rawField, err := f.raw(); err == nil {
	if _, err := w.Write(rawField); err != nil {
		return err
	}
} else {
	return fmt.Errorf("failed to write header field #%v (%q): %w", len(h.l) - i, f.k, err)
}