Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Obsolete ADLER-32 but don't forbid it. It's now NOT RECOMMENDED #828 #877

Merged
merged 1 commit into from Oct 9, 2019

Conversation

@ioggstream
Copy link
Contributor

commented Jul 25, 2019

This PR

Obsolete ADLER-32 but don't forbid it. #828

@ioggstream

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

@manger will this work for you?

@ioggstream ioggstream changed the title Obsolete ADLER-32 but don't forbid it. #828 Obsolete ADLER-32 but don't forbid it. It's now NOT RECOMMENDED #828 Aug 1, 2019
@paulmillar

This comment has been minimized.

Copy link

commented Aug 12, 2019

I think the pull request is OK as-is, but might be improved slightly.

In particularly, I'm wondering if SHOULD NOT is too strong for the server. A server could support ADLER32 without that support impacting on cryptographic data integrity provided that server also supports a cryptographically secure hash; e.g., SHA-512. Clients that wish to protect against malicious actor changes would use the SHA-512 hash and not request (or ignore) the ADLER32 value. Therefore, I would suggest that, for the server, the document says a server MAY implement ADLER32.

I think it is perfectly reasonable for the document to say the client SHOULD NOT use ADLER32, for the reasons stated.

That said, I can't decide if this distinction is really helpful, so I defer to your judgement, here.

@LPardue

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

I'm not so sure we want to make a distinction between client and server as you write it. Both client and server can attach Digest header to request or response. It is the receiver of Digest that is exposed to the risk of using a weak digest.

The current language avoids mentioning either endpoint so we leave it to implementers. I'm not opposed to spelling things out more clearly but we'd really need to nail down what we mean by "using Digest algorithms" because that relates to requesting them, generating them, regenerating them (proxy), validating them (endpoint or proxy) etc.

@paulmillar

This comment has been minimized.

Copy link

commented Aug 12, 2019

That's a good point about client/server. Sorry, I had that wrong. As you say, it's the recipient of a Digest that must decide whether the algorithm is sufficient. This recipient may be either the client or the server.

I think the document is right to point out the dangers of accepting a weak digest.

Perhaps there is a useful distinction between generating a digest and accepting a digest; e.g., an agent MAY support generating a weak digest but SHOULD NOT accept a weak digest.

@LPardue

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

That's a good point about client/server. Sorry, I had that wrong.

Not wrong, just partially true.

Perhaps there is a useful distinction between generating a digest and accepting a digest; e.g., an agent MAY support generating a weak digest but SHOULD NOT accept a weak digest.

That could work for me. I'll leave it to @ioggstream to decide what he things makes sense to do.

@ioggstream

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

@paulmillar @LPardue as this issue is for ADLER, I moved the weak digest discussion to #894

.. A server could support ADLER32 without that support impacting on cryptographic data integrity
[.. ] the document says a server MAY implement ADLER32.

As it's in the listed algorithms, the server MAY already implement ADLER32, though it SHOULD NOT be used by both sides of the communication. I think sender and receiver should behave consistently (see #894 again).

@ioggstream ioggstream requested a review from LPardue Aug 19, 2019
@ioggstream ioggstream force-pushed the 828-other-algorithms branch from 3129eea to 5024628 Oct 9, 2019
@ioggstream ioggstream force-pushed the 828-other-algorithms branch from 5024628 to 5890230 Oct 9, 2019
@ioggstream

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

Merging as for the positive feedback from @phluid61 and @LPardue comment #877 (comment)

@ioggstream ioggstream merged commit 032840d into master Oct 9, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@ioggstream ioggstream deleted the 828-other-algorithms branch Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.