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

Put the DH public key in the message. #9

Merged
merged 3 commits into from
Dec 22, 2016
Merged

Conversation

martinthomson
Copy link
Contributor

This removes Crypto-Key and moves the DH public key into the keyid field.

Copy link
Contributor

@beverloo beverloo left a comment

Choose a reason for hiding this comment

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

The change looks good to me.

I'm still not completely convinced of the benefit, since any message will likely need to include other meta information anyway (e.g. the subscription for which it was received). But I defer to Costin who knows that part of the system much better.

A push message MUST include a zero length `keyid` parameter in the content
coding header.
A push message MUST include the application server ECDH public key in the
`keyid` parameter of the encrypted content coding header.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is the MUST mandate, would it make sense to move the "The uncompressed point...of the "keyid"." language on lines 187-189 of this diff here?

Copy link

Choose a reason for hiding this comment

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

The subscription is encoded in the endpoint URL, 'from' is in the Authorization header.

It would have been nice if the binary format was extensible - I understand the motivation
of keeping it as simple as possible, but some tag/len or other mechanism are not that
complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, extensibility is hazardous without knowing what you need it for. At this point, I'd say that it is best achieved by using an entirely different scheme.

Losing extensibility is one of the costs of this optimization. If you really think we need extensibility, then this PR is a bad idea and we should go back to using Crypto-Key.

Copy link

Choose a reason for hiding this comment

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

It's ok - we can still extend/customize this format by adding info in headers :-)

@martinthomson martinthomson force-pushed the pack_key_hack branch 3 times, most recently from e282eea to 80c46d5 Compare November 26, 2016 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants