- 
                Notifications
    
You must be signed in to change notification settings  - Fork 11
 
Speed up batou age interaction #496
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
base: main
Are you sure you want to change the base?
Conversation
| 
           Calling out to the rage library is probably the right call for performance here. I distinctly remember being very frustrated during development of the age binary integration.  | 
    
| 
           The largest performance hog now is the call out to  Certainly usable but could be significantly improved imho.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to note that 3.7 is no longer supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in eb1bd5f
| """\ | ||
| batou/2... (cpython 3...) | ||
| ================================== Preparing =================================== | ||
| ... Preparing ... | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not quite clear why this changes when changing the encryption?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not directly related to the encryption but was required so I can run the test suite on my machine to test all other changes. The specific commit is d4cacca. I can move these out to a separate PR now if you want.
| 
           I also get an error in our standard deploy container, meaning there is another dependency which might not be available everywhere (think switches). While a remote system does not need do decrypt (since that happens locally) the dependency would be required. We need to re-think this, I suppose.  | 
    
7b7b6f5    to
    ba432a8      
    Compare
  
    | 
           Yes, the added dependency on pyrage does require a rust compiler if there is no wheel available for that platform. We could gate the depedency on pyrage behind a feature flag that is enabled by default so it can be turned off for some systems 🤔  | 
    
        
          
                src/batou/secrets/encryption.py
              
                Outdated
          
        
      | key_content = f.read() | ||
| try: | ||
| priv_key = serialization.load_ssh_private_key(key_content, None) | ||
| except ValueError: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually should be
| except ValueError: | |
| except (ValueError, TypeError): | 
I ran into an issue where batou secrets edit staging failed with the error message thrown inside cryptography
Key is password-protected, but password was not provided.
that is a TypeError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add, thanks for the review. I did not consider that case.
If only it was clear what kinds of exceptions could be thrown when calling a function...
eb1bd5f    to
    844647f      
    Compare
  
    | 
           I will attempt to loosen the dependency on pyrage by adding a bit of yes/no logic in the encryption code such that systems that either don't need pyrage at all or don't have a pre-built pyrage wheels available are not deprecated with this PR  | 
    
54e8738    to
    f7d4c98      
    Compare
  
    Massively improve secret handling performance by avoiding large amounts of writing to / reading from temporary files. This solves a lot of complexity around the age call because it does not require piping the ssh key's password into a subprocess' stdin conditionally. Instead, this can be handled using `cryptography` to handle the password-protected ssh key and `pyrage`, python bindings to the rage library which supports de/encrypting in-memory. Another benefit of this implementation is a relaxed requirement (now optional) on the `age` CLI since the bindings to the rage library provide a full implementation out of the box.
The test suite provides some git configuration, additional custom git configuration should not cause test cases to fail. GPG has a maximum length for the path to the GPG agent socket, Copying the entire gpg directory to a temporary directory ensures that the path is not too long when the repo is cloned into a somewhat nested directory structure.
recipient errors can occur when using rsa keys with module > 4096 as recipients with rage, age can handle those
679e1cb    to
    664b79c      
    Compare
  
    
The most relevant commit is 8d569b2.
Due to the previous implementation working with generous amounts of temporary files (especially so when using
age-diffable), the IO overhead from reading from and writing to these files is large enough to severely slow down secret handling operations.This PR implements in-memory handling of deployment secrets via bindings to the rage library, a rust implementation of the age spec. This leads to massively improved secret handling performance.
While this PR does introduce some added requirements on external libraries, it also relaxes the dependence on the (r)age cli.
When implementing this PR, I discovered a few bugs in the test implementation such as improper isolation of calls to
gitandgpghaving issues creating an agent socket in nested folder structures as well as a dependence on a very specific terminal width where a bunch of tests would fail due to very strict string comparison. I have added these changes to this PR.N.B.: due to limitations around handling of RSA keys in the cryptography library that rage depends on, RSA keys with moduli > 4096 are (currently) not supported via the bindings. In these cases, the implementation attempts to fall back to calling the
agebinary - see commit 000b98a. Sincerageis incompatible with these RSA keys anyways, the previous command discovery implementation could be simplified.