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

Redact ?token= querystring parameter from GHES <3.8 archive URLs in logs #1152

Merged
merged 8 commits into from
Nov 10, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion RELEASENOTES.md
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@

- Skip GitHub status checking on startup if environment variable `GEI_SKIP_STATUS_CHECK` is set to true.
- Skip GitHub status checking on startup if environment variable `GEI_SKIP_STATUS_CHECK` is set to `true`
- Redact `?token=` querystring parameter from GHES <3.8 archive URLs in logs
22 changes: 17 additions & 5 deletions src/Octoshift/Services/OctoLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System.IO;
using System.Linq;
using System.Net.Http;
using System.Text.RegularExpressions;

namespace OctoshiftCLI.Services;

Expand All @@ -18,6 +19,7 @@ internal static class LogLevel
public class OctoLogger
{
public virtual bool Verbose { get; set; }
private readonly HashSet<Regex> _redactionPatterns = new();
private readonly HashSet<string> _secrets = new();
private readonly string _logFilePath;
private readonly string _verboseFilePath;
Expand Down Expand Up @@ -59,7 +61,7 @@ public OctoLogger(Action<string> writeToLog, Action<string> writeToVerboseLog, A
private void Log(string msg, string level)
{
var output = FormatMessage(msg, level);
output = MaskSecrets(output);
output = Redact(output);
if (level == LogLevel.ERROR)
{
_writeToConsoleError(output);
Expand All @@ -78,7 +80,7 @@ private string FormatMessage(string msg, string level)
return $"[{timeFormat}] [{level}] {msg}\n";
}

private string MaskSecrets(string msg)
private string Redact(string msg)
{
var result = msg;

Expand All @@ -88,6 +90,11 @@ private string MaskSecrets(string msg)
.Replace(Uri.EscapeDataString(secret), "***");
}

foreach (var redactionPattern in _redactionPatterns)
{
result = redactionPattern.Replace(result, "***");
}

return result;
}

Expand Down Expand Up @@ -117,14 +124,14 @@ public virtual void LogError(Exception ex)
var verboseMessage = ex is HttpRequestException httpEx ? $"[HTTP ERROR {(int?)httpEx.StatusCode}] {ex}" : ex.ToString();
var logMessage = Verbose ? verboseMessage : ex is OctoshiftCliException ? ex.Message : GENERIC_ERROR_MESSAGE;

var output = MaskSecrets(FormatMessage(logMessage, LogLevel.ERROR));
var output = Redact(FormatMessage(logMessage, LogLevel.ERROR));

Console.ForegroundColor = ConsoleColor.Red;
_writeToConsoleError(output);
Console.ResetColor();

_writeToLog(output);
_writeToVerboseLog(MaskSecrets(FormatMessage(verboseMessage, LogLevel.ERROR)));
_writeToVerboseLog(Redact(FormatMessage(verboseMessage, LogLevel.ERROR)));
}

public virtual void LogVerbose(string msg)
Expand All @@ -137,7 +144,7 @@ public virtual void LogVerbose(string msg)
}
else
{
_writeToVerboseLog(MaskSecrets(FormatMessage(msg, LogLevel.VERBOSE)));
_writeToVerboseLog(Redact(FormatMessage(msg, LogLevel.VERBOSE)));
}
}

Expand All @@ -157,4 +164,9 @@ public virtual void LogSuccess(string msg)
}

public virtual void RegisterSecret(string secret) => _secrets.Add(secret);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also avoid this refactor, and keep a separate list of _secrets which we replace as well as the "redaction patterns". That's probably a safer approach to avoid surprises - but I'd love to hear what y'all think.


public virtual void AddRedactionPattern(Regex pattern)
{
_redactionPatterns.Add(pattern);
}
}
26 changes: 26 additions & 0 deletions src/OctoshiftCLI.Tests/Octoshift/Services/OctoLoggerTests.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Net;
using System.Net.Http;
using System.Text.RegularExpressions;
using FluentAssertions;
using OctoshiftCLI.Services;
using Xunit;
Expand Down Expand Up @@ -60,6 +61,31 @@ public void Secrets_Should_Be_Masked_From_Logs_And_Console()
_consoleError.Should().NotContain(urlEncodedSecret);
}

[Fact]
public void Redaction_Patterns_Should_Be_Replaced_In_Logs_And_Console()
{
var password = "hunter2";

_octoLogger.AddRedactionPattern(new Regex("hunter\\d", RegexOptions.IgnoreCase));

_octoLogger.Verbose = false;
_octoLogger.LogInformation($"Don't tell anyone that {password} is my password");
_octoLogger.LogVerbose($"Don't tell anyone that {password} is my password");
_octoLogger.LogWarning($"Don't tell anyone that {password} is my password");
_octoLogger.LogSuccess($"Don't tell anyone that {password} is my password");
_octoLogger.LogError($"Don't tell anyone that {password} is my password");
_octoLogger.LogError(new OctoshiftCliException($"Don't tell anyone that {password} is my password"));
_octoLogger.LogError(new InvalidOperationException($"Don't tell anyone that {password} is my password"));

_octoLogger.Verbose = true;
_octoLogger.LogVerbose($"Don't tell anyone that {password} is my password");

_consoleOutput.Should().NotContain(password);
_logOutput.Should().NotContain(password);
_verboseLogOutput.Should().NotContain(password);
_consoleError.Should().NotContain(password);
}

[Fact]
public void LogError_For_OctoshiftCliException_Should_Log_Exception_Message_In_Non_Verbose_Mode()
{
Expand Down
3 changes: 3 additions & 0 deletions src/gei/Commands/MigrateRepo/MigrateRepoCommand.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.CommandLine;
using System.Text.RegularExpressions;
using Microsoft.Extensions.DependencyInjection;
using OctoshiftCLI.Commands;
using OctoshiftCLI.Contracts;
Expand Down Expand Up @@ -188,6 +189,8 @@ public override MigrateRepoCommandHandler BuildHandler(MigrateRepoCommandArgs ar
var ghesVersionChecker = ghesVersionCheckerFactory.Create(ghesApi);
var warningsCountLogger = sp.GetRequiredService<WarningsCountLogger>();

log.AddRedactionPattern(new Regex("\\?token=[A-Z0-9]{29}"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have manually tested that this works as expected on GHES 3.7, and the URL is not logged.


return new MigrateRepoCommandHandler(
log,
ghesApi,
Expand Down