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

[aes] Add signature conversion decorator to aes #29973

Closed
wants to merge 4 commits into from
Closed

Conversation

sulyi
Copy link

@sulyi sulyi commented Sep 15, 2021

Before submitting a pull request make sure you have:

In order to be accepted and merged into youtube-dl each piece of code must be in public domain or released under Unlicense. Check one of the following options:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

  • Bug fix
  • Improvement
  • New extractor
  • New feature

Description of your pull request and other information

It starts an overhaul of aes.

  • Simplifies signature of functions in the module, instead list of integers they accept bytes.
  • Adds function for AES-128-GCM decrypt (uses already existing function for AES-CRT) and verification.
  • Adds new functionality to existing code (i.e. AES-CRT nonce)
  • Converts (internal) functions to also use mentioned types.
  • Restructures the code according to OOP design
  • Adds new tests cases to test_aes to safeguard code

I'd like to mention @pukkandan and the help he gave me during the development of this code at yt-dlp/yt-dlp#938.

@sulyi
Copy link
Author

sulyi commented Sep 15, 2021

I have the test just haven't added them yet.

@dirkf
Copy link
Contributor

dirkf commented Sep 19, 2021

Why have the aes_... functions been given a leading _, contrary to PEP8?

Is this halfway to refactoring the code with these functions as static methods of some class?

The list __all__ of functions in the module aes.py doesn't have the _.

@sulyi
Copy link
Author

sulyi commented Sep 19, 2021

Why have the aes_... functions been given a leading _, contrary to PEP8?

There's no contradiction here, they are intended yo be private.

Is this halfway to refactoring the code with these functions as static methods of some class?

I wouldn't use static but, kind of yes. Moving functions into a class is secondary, but rewriting them to use byte instead of list of ints is.

The list all of functions in the module aes.py doesn't have the _.

Private functions are exposed here.

Comment on lines +122 to +124
def _aes_gcm_decrypt_and_verify(data, key, tag, nonce):
"""
Generate key schedule
Decrypt with aes in GBM mode and checks authenticity using tag
Copy link
Contributor

Choose a reason for hiding this comment

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

Since #29201 is not merged, youtube-dl does not have any need for gcm

Copy link
Author

Choose a reason for hiding this comment

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

True, since I did it I just want it out somewhere...

@dirkf
Copy link
Contributor

dirkf commented Sep 19, 2021

OK, now I see the new block (which after all is mentioned in the title of the PR!) that all makes sense, thanks.

Making the lists of int internal certainly seems a good idea. The decorator mechanism is slick and functional, and systematises the code, even if it doesn't always reduce it by much. Eg AIUI these are equivalent?

@signature_conversion(convert_all_args_to_intlist, None, intlist_to_bytes)
def aes_ctr_decrypt(data, key, iv):
    return _aes_ctr_decrypt(data, key, iv)

vs

def aes_ctr_decrypt(data, key, iv):
    return intlist_to_bytes(
        _aes_ctr_decrypt(bytes_to_intlist(data), bytes_to_intlist(key), bytes_to_intlist(iv)))

But some of the other interface signatures need more complex munging and so should be more simply expressed by the decorator.

@sulyi
Copy link
Author

sulyi commented Sep 19, 2021

Nope, that's is done here and here.

But, @dirkf, you got me thinking whether it is possible to slap decorator directly on original functions.

@pukkandan
Copy link
Contributor

you got me thinking whether it is possible to slap decorator directly on original functions.

If you never need access to the original functions (even internally), yes. You can just put the decorator on the original without needing a wrapper

@sulyi
Copy link
Author

sulyi commented Oct 1, 2021

My original assumption that using bytes would be better is a bust: https://bugs.python.org/issue19251.
But I was yet able to make some improvements, and imho, the new signature is still friendlier.

@sulyi sulyi closed this May 6, 2022
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