Skip to content

Conversation

corbob
Copy link
Member

@corbob corbob commented Oct 28, 2024

Description Of Changes

Better handle failures to decrypt the arguments file.

Motivation and Context

When the file fails to decrypt, it should not block things unless it is actually a blocking failure.

Testing

Note: for my testing I downloaded the internal build from our build server. If you build these components yourself you'll need to use --allow-unofficial on all of the choco commands.

  1. Install Chocolatey package for this PR.
  2. Install at least 2 packages (choco install glab -y will install glab and glab.portable)
  3. Navigate to C:\ProgramData\Chocolatey\.chocolatey
  4. For each of the packages just installed, locate the directory and open the .arguments file within. (You'll need to run your editor as admin)
  5. In one of the files add a non-base64 character (any symbol other than + or / should suffice. I added ! to the start of the glab file.)
  6. In the other file replace it's entire contents with AQAAANCMnd8BFdERjHoAwE/Cl+sBAAAAn1/taDnOFUqGb17fBymxHQQAAAACAAAAAAAQZgAAAAEAACAAAAAU8gmqznJYKdkuj8bgk8sgg6Le3sbGoGkZOV3YtRFfwwAAAAAOgAAAAAIAACAAAAD1I9LYxrEhx9m71eF3VqyAike+XJTePhDAcrOilAFjQlAAAAA8lfiMR5Ns/AntLdVR3eBQSduCnipRCbdu/er/+YABMTzJDMGqnXuIsKwWoNIhrB14Yit4jVPipt3a/Nx18xx+YsnUewI4P6GlDL5do1y8mkAAAABMxvyPgCtN36BwAOXvJghIh9Hs8jUZOJtQIlWci8BnJkBmaaoSZ6pTGULk4TbFXMf/FK1NPo2mPM0YVL8QgJyK (this is the string used in the text. It is from a VM that has long been discarded).
  7. Run the following test commands all of which should succeed:
    1. choco list
    2. choco pin list
    3. choco pin add -n glab
    4. choco pin list
    5. choco pin remove -n glab
    6. choco install gitbutler -y
    7. choco upgrade all -y (note this is not expected to find any updates, but it also does not error)
  8. Run the following test commands which should succeed, but should also output a warning:
    1. choco list --verbose
    2. choco info glab --local-only
    3. choco info glab.portable --local-only
  9. Verify the warnings of the above commands are in the form of:
    There were some failures decrypting package arguments.
    Failed packages: glab - 1.48.0, glab.portable - 1.48.0
    
  10. Open the Chocolatey log and verify the previous commands emitted debug entries in the form of:
    2024-10-30 09:53:46,577 8396 [DEBUG] - There was an error attempting to decrypt the contents of the .arguments file for version '1.48.0' of package 'glab.portable'.  See log file for more information.
    # The below is for the package with the invalid encryption:
    2024-10-30 09:53:46,577 8396 [DEBUG] - We failed to decrypt C:\ProgramData\chocolatey\.chocolatey\glab.portable.1.48.0\.arguments. Error from decryption: Key not valid for use in specified state.
    # The below is for the invalid base64 character:
    2024-10-30 09:53:38,033 3536 [DEBUG] - We failed to decrypt C:\ProgramData\chocolatey\.chocolatey\glab.1.48.0\.arguments. Error from decryption: The input is not a valid Base-64 string as it contains a non-base 64 character, more than two padding characters, or an illegal character among the padding characters. 
    
  11. Enable useRememberedArgumentsForUpgrades feature: choco feature enable -n useRememberedArgumentsForUpgrades
  12. Run the following test commands which should fail:
    1. choco upgrade glab -y
      There was an error attempting to decrypt the contents of the .arguments file for version '1.48.0' of package 'glab'.  See log file for more information.
      We failed to decrypt C:\ProgramData\chocolatey\.chocolatey\glab.1.48.0\.arguments. Error from decryption: The input is not a valid Base-64 string as it contains a non-base 64 character, more than two padding characters, or an illegal character among the padding characters.
      
    2. choco upgrade glab.portable -y
      There was an error attempting to decrypt the contents of the .arguments file for version '1.48.0' of package 'glab'.  See log file for more information.
      We failed to decrypt C:\ProgramData\chocolatey\.chocolatey\glab.1.48.0\.arguments. Error from decryption: Key not valid for use in specified state.
      
  13. Run the following command which should succeed:
    6. choco uninstall glab -y
    7. choco uninstall glab.portable -y
    8. choco upgrade gitbutler -y

In addition to the above testing, I have also tested this in Test Kitchen against both this PR and against 2.3.0 to validate that the tests fail on 2.3.0

Operating Systems Testing

  • Windows 10 22H2
  • Windows Server 2022 - Repository Pester Testing Vagrantfile
  • Windows Server 2016 - Test Kitchen tests
  • Windows Server 2019 - Test Kitchen tests

Change Types Made

  • Bug fix (non-breaking change).
  • Feature / Enhancement (non-breaking change).
  • Breaking change (fix or feature that could cause existing functionality to change).
  • Documentation changes.
  • PowerShell code changes.

Change Checklist

  • Requires a change to the documentation.
  • Documentation has been updated.
  • Tests to cover my changes, have been added.
  • All new and existing tests passed?
  • PowerShell code changes: PowerShell v3 compatibility checked?

Related Issue

@corbob
Copy link
Member Author

corbob commented Oct 28, 2024

This is WIP as I still need to add testing steps, as well as some tests to the repository. I also need to validate that it does fix 3513 as right now it's just suspicions that it will.

@corbob corbob force-pushed the handle-decryption-failures branch 3 times, most recently from d183bb2 to 8248dd0 Compare October 30, 2024 17:26
@corbob corbob requested review from vexx32 and gep13 October 30, 2024 17:27
@corbob corbob marked this pull request as ready for review October 30, 2024 17:28
@corbob
Copy link
Member Author

corbob commented Oct 30, 2024

This PR is now ready for review.

@corbob corbob self-assigned this Oct 30, 2024
@corbob corbob marked this pull request as draft October 30, 2024 17:59
@corbob corbob added the Requires Upstream Change Requires changes to a different location once issue is fixed or implemented label Oct 30, 2024
@corbob
Copy link
Member Author

corbob commented Oct 30, 2024

I'm marking this as draft again so we don't merge it before we have some internal discussions about this. This appears that it may require changes in other projects, and we'll need to understand the repercussions of those changes.

@corbob corbob marked this pull request as ready for review October 30, 2024 19:50
@corbob
Copy link
Member Author

corbob commented Oct 30, 2024

ok, back to ready for review as we now understand the changes.

Copy link
Member

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

Couple minor things we should probably clean up but otherwise this looks good to me, great work!

@corbob corbob force-pushed the handle-decryption-failures branch from 8248dd0 to ff7f4cf Compare November 4, 2024 20:05
@corbob corbob changed the title Handle arguments file decryption failures (#3503, #3513) Handle arguments file decryption failures Nov 4, 2024
@corbob corbob force-pushed the handle-decryption-failures branch 2 times, most recently from c09b8c5 to 8399dcd Compare November 4, 2024 21:07
Copy link
Member

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

I think this looks good here, great work! 💜

Currently we are decrypting the arguments file every time a list is run,
even though we only care about them in scenarios where we might output
them to the user (when Verbose is specified on the list command or with
the info --local-only command). This updates the handling of the
remembered arguments to be consistent between the List method and the
scenario in an Upgrade where we want to use the remembered arguments. We
now will only throw an error in a scenario where we can't decrypt the
contents and we need them for the scenario to succeed. We are also
stating what file we couldn't decrypt so the user can troubleshoot.
Add some Pester tests for the arguments file decryption handling.
Ensuring that both invalid keys and base64 corruption works correctly.
- Use OrdinalIgnoreCase for command name comparison.
- Use explicitly named parameters instead of true/false.
- Remove comment that's no longer valid.
@corbob corbob force-pushed the handle-decryption-failures branch from 8399dcd to 68b7916 Compare November 4, 2024 22:52
@vexx32 vexx32 merged commit 242b3fc into chocolatey:develop Nov 5, 2024
5 checks passed
@corbob corbob deleted the handle-decryption-failures branch November 5, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Requires Upstream Change Requires changes to a different location once issue is fixed or implemented
Projects
None yet
2 participants