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

feat: Drop aesgcm128 support #268

Merged
merged 3 commits into from
Apr 1, 2021
Merged

feat: Drop aesgcm128 support #268

merged 3 commits into from
Apr 1, 2021

Conversation

jrconlin
Copy link
Member

Description

Drops support for legacy aesgcm128 encryption format. This is in parallel with mozilla/rust-ece#53 which will drop support for the format for mobile devices.

We're also seeing zero messages containing this form, so it's absolutely safe to drop it.

Testing

messages encoded with aesgcm128 should be rejected.

Issue

Closes: #266

@jrconlin jrconlin requested a review from a team March 23, 2021 20:56
@pjenvey
Copy link
Member

pjenvey commented Mar 24, 2021

We're also seeing zero messages containing this form, so it's absolutely safe to drop it.

It's not quite zero but it's very low now, not even 1-2 requests per second

Copy link
Member

@pjenvey pjenvey left a comment

Choose a reason for hiding this comment

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

aesgcm128 appears to be the sole user of the Encryption-Key header. We could also further remove it, killing the encryption_key field in here and related enckey entry in the router message data.

The only reference to it we might need going forward is the explicit invalidation if it was included in the request. This error's only triggered for the 04 encoding but I don't see why it couldn't apply to 06 too. If so, the check could occur in from_request instead so the struct field wouldn't be needed.

Maybe we don't even need the check any longer either?

I'm happy to defer this till later but I think it's worth the effort even for such a minor cleanup -- otherwise this field's likely to linger around forever. The various bits of the different crypto standards are complicated enough as is.

@jrconlin jrconlin merged commit d8b7ca8 into master Apr 1, 2021
@jrconlin jrconlin deleted the feat/266-drop-aesgcm128 branch April 1, 2021 23:12
@jrconlin jrconlin mentioned this pull request Jun 2, 2021
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.

Drop support for aesgcm128
2 participants