Opt-in to API key forwarding. Default to using SeqCli's connection se…#406
Conversation
| static readonly Encoding Utf8 = new UTF8Encoding(false); | ||
|
|
||
| readonly ForwardingChannelMap _forwardingChannels; | ||
| private readonly SeqCliConfig _config; |
There was a problem hiding this comment.
Nit - explicit private modifier
| public uint? PooledConnectionLifetimeMilliseconds { get; set; } = null; | ||
| public ulong EventBodyLimitBytes { get; set; } = 256 * 1024; | ||
| public ulong PayloadLimitBytes { get; set; } = 10 * 1024 * 1024; | ||
| public bool ForwardApiKey { get; set; } |
There was a problem hiding this comment.
I think this should be in SeqCliForwarderConfig
There was a problem hiding this comment.
How about UseApiKeyForwarding? Enable sounds a bit like it is optional.
| var log = _forwardingChannels.Get(GetApiKey(context.Request)); | ||
| var log = _forwardingChannels.Get(_config.Connection.ForwardApiKey | ||
| ? GetApiKey(context.Request) | ||
| : null); |
There was a problem hiding this comment.
null will get the default channel; when ForwardApiKey is false, the default channel should not inherit the default key?
| return _defaultChannel; | ||
| foreach (var directoryPath in Directory.EnumerateDirectories(_bufferPath)) | ||
| { | ||
| if (directoryPath.Equals(GetStorePath("Default"))) continue; |
There was a problem hiding this comment.
Nit: could use DefaultChannelName here?
| { | ||
| var created = OpenOrCreateChannel(apiKey, ApiKeyToName(apiKey)); | ||
|
|
||
| lock (_channelsSync) |
There was a problem hiding this comment.
Since this is only called in the constructor where there's no parallelism, it might be best to omit the lock statement here so the code doesn't give the impression of there being some.
Renaming the method to LoadApiKeyChannels or InitApiKeyChannels or OpenApiKeyChannels might communicate its role better?
| { | ||
| // Seq API keys begin with four identifying characters that aren't considered part of the | ||
| // confidential key. TODO: we could likely do better than this. | ||
| return apiKey[..(Math.Min(apiKey.Length, 4))]; |
There was a problem hiding this comment.
On the next pass, if we fix/improve the directory naming scheme, we should include a min-length requirement and ignore payloads/keys below it. Seq's minimum is something like 16 chars IIRC.
|
|
||
| public void WriteApiKey(SeqCliConfig config, string apiKey) | ||
| { | ||
| File.WriteAllBytes(Path.Combine(_directoryPath, "api.key"), Encoding.UTF8.GetBytes(apiKey)); |
There was a problem hiding this comment.
Missing the Encrypt() step here?
…y-forwarding # Conflicts: # src/SeqCli/Forwarder/Web/Api/IngestionEndpoints.cs
| return Path.Combine(_bufferPath, name); | ||
| } | ||
|
|
||
| public ForwardingChannel GetApiKeyForwardingChannel(string? requestApiKey) |
There was a problem hiding this comment.
This should deny the use of SeqCliConnectionChannelName to steer clear of any possible future vector for compromise
| { | ||
| // Seq API keys begin with four identifying characters that aren't considered part of the | ||
| // confidential key. TODO: we could likely do better than this. | ||
| return string.IsNullOrEmpty(apiKey) ? "EmptyApiKey" : apiKey[..(Math.Min(apiKey.Length, 4))]; |
There was a problem hiding this comment.
Perhaps EmptyApiKey should also get a constant, and we should also disallow its use as an argument to GetApiKeyForwardingChannel?
There was a problem hiding this comment.
EmptyApiKey needs to work with GetApiKeyForwardingChannel since it is part of the forwarding API key path.
There was a problem hiding this comment.
GetApiKeyForwardingChannel accepts null in the case of an empty/missing API key. The case I'm thinking of is where an incoming request uses apiKey=EmptyApiKey, which we could bounce in a precondition of GetEmptyApiKeyForwardingChannel.
| stopChannels = _channelsByName.Values.Select(ch => ch.StopAsync()).ToArray(); | ||
| } | ||
|
|
||
| if (_seqCliConnectionChannel != null) |
There was a problem hiding this comment.
Should the lock block above be in an else branch following this block? The concatentation suggests the two cases might occur together, but the comment at the top lays out an invariant that it's either one or the other.
| config.Encryption.DataProtector().Encrypt(Encoding.UTF8.GetBytes(apiKey))); | ||
| } | ||
|
|
||
| public string? ReadApiKey(SeqCliConfig config) |
| } | ||
|
|
||
| var path = new SystemStoreDirectory(directoryPath); | ||
| var apiKey = path.ReadApiKey(_config); |
There was a problem hiding this comment.
If the read fails, here, perhaps we should skip rather than fall through? Effects are going to be weird otherwise - e.g. the Add() call on 74 will fail on subsequent keys that also fail.
…y-forwarding # Conflicts: # src/SeqCli/Forwarder/Channel/ForwardingChannelMap.cs # src/SeqCli/Forwarder/ForwarderModule.cs # src/SeqCli/Forwarder/Web/Api/IngestionEndpoints.cs
…y-forwarding # Conflicts: # src/SeqCli/Forwarder/Channel/ForwardingChannelMap.cs
2 participants
If
useApiKeyForwardingis on then events will always be sent with the arriving API key. Events buffered whenuseApiKeyForwardingis off can only be sent whenuseApiKeyForwardingis off.