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

Removing umlaut limitation in secrets #157

Closed
wants to merge 12 commits into from

Conversation

juliandittmann
Copy link

Intro:

Secrets with umlauts were forbidden due to #131.

Change:

I removed the limitation by converting secrets to base64 and back.
The idea was mentioned by @freddydk in #131.

I am looking for a codereview.

@ghost
Copy link

ghost commented Jun 8, 2022

CLA assistant check
All CLA requirements met.

@freddydk
Copy link
Collaborator

freddydk commented Jun 8, 2022

Yeah, I tried something similar, but was unable to find a mechanism where the secret isn't exposed.
The big problem is really that when the secret is used later - it might be exposed in the log - and since we need the secret as non-base64. In your case - it might be that it doesn't show up exactly in this area - but then it might show up in a different area.
And since exposing secrets might reveal classified information, I found no other way than blocking secrets with special characters and I am afraid that this PR doesn't solve the problem fundamentally.

The problem (as I see it) is a bug in GitHub.

@juliandittmann
Copy link
Author

juliandittmann commented Jun 9, 2022

I also think it is definitely a bug in GitHub. I tracked down the problem to yaml.

SecretsJson: ${{ env.RepoSecrets }}

The secrets are correctly formatted in env.RepoSecrets. After they are passed to the action it breaks. I dont think i can fix the Github bug soon.

Another idea would be handling the secrets with outputs. Also just a workaround. Other actions for example Azure/get-keyvault-secrets have taken this way. I think it is quite unhandy to manage secrets like this and it is lacking extensibility.

Note: I obviously have secrets with umlauts that i can not change anymore and it is not the licensefileUrl.

@freddydk
Copy link
Collaborator

freddydk commented Jun 9, 2022

Which secret are we talking about?
Maybe we can allow specific secrets to be base64 encoded always - until usage

@juliandittmann
Copy link
Author

My codeSignCertificatePassword contains umlauts.

Meanwhile i submitted the bug to GitHub. I will post updates here?

@freddydk
Copy link
Collaborator

freddydk commented Jun 9, 2022

That is a real concern - I will identify a solution asap

@freddydk
Copy link
Collaborator

freddydk commented Jun 9, 2022

What you can do right now is to create a file called SignBcContainerApp.ps1 and place it in the .AL-Go folder of your repo/project and place this code in the file:

Param(
    [Hashtable]$parameters
)

$parameters.pfxPassword = ConvertTo-SecureString -String ([System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String(($parameters.pfxPassword | Get-PlainText)))) -AsPlainText -Force
Sign-BcContainerApp @parameters

Then you can create your password secret with base64 encoding as the Sign-BcContainerApp in your repo now assumes that the password is base64 encoded.

I might find a better solution, but this should unblock you.

@juliandittmann
Copy link
Author

Update from GitHub support:

Thank you for reaching out to GitHub Support! I apologize for the delayed response here.

I did a bit of testing with this scenario today and noticed that the behavior you are experiencing with umlauts is specific to Powershell. The following Actions workflow outputs characters in powershell, which looks to return various symbols instead of the umlauts - just like you are seeing in your workflow:

on: [push]

defaults:
  run:
    shell: powershell

jobs:
  output:
    runs-on: windows-latest
    steps:
      - run: echo 'aäoöuü'
> Run echo 'aäoöuü'
aäoöuü 

From some other posts I found regarding this interaction, it appears that powershell's InputEncoding and OutputEncoding need to be configured to UTF-8, which looks to be the default on the Actions runner:

>$OutputEncoding = [console]::OutputEncoding
>$InputEncoding = [console]::InputEncoding
>  
>echo $OutputEncoding
>echo $InputEncoding

BodyName          : utf-8
EncodingName      : Unicode (UTF-8)
HeaderName        : utf-8
WebName           : utf-8
WindowsCodePage   : 1200
IsBrowserDisplay  : True
IsBrowserSave     : True
IsMailNewsDisplay : True
IsMailNewsSave    : True
IsSingleByte      : False
EncoderFallback   : System.Text.EncoderReplacementFallback
DecoderFallback   : System.Text.DecoderReplacementFallback
IsReadOnly        : True
CodePage          : 65001

BodyName          : utf-8
EncodingName      : Unicode (UTF-8)
HeaderName        : utf-8
WebName           : utf-8
WindowsCodePage   : 1200
IsBrowserDisplay  : True
IsBrowserSave     : True
IsMailNewsDisplay : True
IsMailNewsSave    : True
IsSingleByte      : False
EncoderFallback   : System.Text.EncoderReplacementFallback
DecoderFallback   : System.Text.DecoderReplacementFallback
IsReadOnly        : True
CodePage          : 65001

I'm still going to look a bit into what can be done at the workflow-level to potentially set the necessary encoding, but this doesn't look to be exclusive to Actions, just Powershell.

@freddydk
Copy link
Collaborator

If I try this in various PowerShell "runners" on my laptop:
image
and in PowerShell ISE:
image
and in VS Code
image

@freddydk
Copy link
Collaborator

an interesting observation in a workflow:

Write-Host with a literal in code does not work:
image

If the literal is Base64 - then the output works:
image

So, it doesn't seem to be an output problem - the string seems to have a wrong value in the inline script

@freddydk
Copy link
Collaborator

Now I think I know what is wrong...
image
Whenever you use string literals in PowerShell, then the string becomes the binary representation of the UTF8 string.
The string contains 9 bytes (as chars) which are the 9 bytes, which the UTF8 string consists of.

I might be able to create a fix for this and allow secrets with special characters.

@freddydk
Copy link
Collaborator

I actually think that your original suggestion of carrying the secrets as base64 encoded is good and likely the right approach.
It just couldn't stand alone, we also had to escape some characters.
I was then led off in all kinds of directions due to other challenges with encoding/decoding.

@juliandittmann
Copy link
Author

I also went full cycle and I am not yet satisfied.
I will keep in contact with GitHub support. Perhaps a solution will emerge there.
Your investigation was very helpful to mine. I'll probably sink a few more hours into it.

Meanwhile I updated the pullrequest.

@freddydk
Copy link
Collaborator

You found most places:-)

I have submitted PR to main with support for this - running end 2 end tests now to see if anything breaks.
Will deploy this to preview if/when all tests are passing.

@juliandittmann
Copy link
Author

Implemented with #166

@freddydk
Copy link
Collaborator

Thanks for your investigations, persistence and ideas - as you can see - your original PR was the right idea:-)

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.

None yet

2 participants