Skip to content

Commit b69c471

Browse files
joelverhagenjoelverhagen
authored
Require event_name from GitHub Actions token and add configurable ban list (#10665)
1 parent ce2ec87 commit b69c471

5 files changed

Lines changed: 105 additions & 31 deletions

File tree

‎src/NuGet.Services.Configuration/StringArrayConverter.cs‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,15 @@
1-
// Copyright (c) .NET Foundation. All rights reserved.
1+
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
55
using System.ComponentModel;
66
using System.Globalization;
7+
using System.Linq;
78

89
namespace NuGet.Services.Configuration
910
{
1011
public class StringArrayConverter : ArrayConverter
1112
{
12-
private static readonly string[] EmptyArray = new string[0];
13-
1413
public override bool CanConvertFrom(ITypeDescriptorContext context, Type sourceType)
1514
{
1615
if (sourceType == typeof(string))
@@ -23,16 +22,20 @@ public override bool CanConvertFrom(ITypeDescriptorContext context, Type sourceT
2322

2423
public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, object value)
2524
{
26-
var s = value as string;
27-
if (s != null)
25+
if (value is null)
26+
{
27+
return Array.Empty<string>();
28+
}
29+
30+
if (value is string s)
2831
{
29-
if (s == string.Empty)
32+
if (string.IsNullOrWhiteSpace(s))
3033
{
31-
return EmptyArray;
34+
return Array.Empty<string>();
3235
}
3336
else
3437
{
35-
return s.Split(';');
38+
return s.Split(';').Select(x => x.Trim()).Where(x => x.Length > 0).ToArray();
3639
}
3740
}
3841

‎src/NuGetGallery.Services/Authentication/Federated/FederatedCredentialConfiguration.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,18 +42,30 @@ public interface IFederatedCredentialConfiguration
4242
/// Values are separated by a semicolon when provided in the configuration file.
4343
/// </summary>
4444
string[] AllowedEntraIdTenants { get; }
45+
46+
/// <summary>
47+
/// Event names in GitHub Actions that are not allowed to be used for OIDC tokens.
48+
/// This act as an additional safeguard against misconfigured workflows.
49+
/// Full list of events: https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows
50+
/// </summary>
51+
52+
string[] BannedGitHubActionsEvents { get; }
4553
}
4654

4755
public class FederatedCredentialConfiguration : IFederatedCredentialConfiguration
4856
{
4957
public bool EnableTokenApi { get; set; }
5058

5159
public string? EntraIdAudience { get; set; }
60+
5261
public string? NuGetAudience { get; set; }
5362

5463
public TimeSpan ShortLivedApiKeyDuration { get; set; } = TimeSpan.FromMinutes(15);
5564

5665
[TypeConverter(typeof(StringArrayConverter))]
5766
public string[] AllowedEntraIdTenants { get; set; } = [];
67+
68+
[TypeConverter(typeof(StringArrayConverter))]
69+
public string[] BannedGitHubActionsEvents { get; set; } = [];
5870
}
5971
}

‎src/NuGetGallery.Services/Authentication/Federated/GitHubTokenPolicyValidator.cs‎

Lines changed: 43 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System;
55
using System.Data.Entity.Infrastructure;
66
using System.IO;
7+
using System.Linq;
78
using System.Threading.Tasks;
89
using Microsoft.IdentityModel.JsonWebTokens;
910
using Microsoft.IdentityModel.Protocols;
@@ -35,6 +36,7 @@ public class GitHubTokenPolicyValidator : TokenPolicyValidator
3536
private const string EnvironmentClaim = "environment";
3637
private const string RepositoryOwnerIdClaim = "repository_owner_id";
3738
private const string RepositoryIdClaim = "repository_id";
39+
private const string EventNameClaim = "event_name";
3840
private const string HttpsPrefix = "https://";
3941
private const string GitHubPrefix = "github.com/";
4042
private const string WorkflowPrefix = ".github/workflows/";
@@ -177,10 +179,41 @@ public override async Task<FederatedCredentialPolicyResult> EvaluatePolicyAsync(
177179
return FederatedCredentialPolicyResult.NotApplicable;
178180
}
179181

182+
// Check for required claims
183+
string? error = TryGetRequiredClaim(jwt, RepositoryClaim, out _);
184+
if (error != null)
185+
{
186+
return FederatedCredentialPolicyResult.Unauthorized(error, isErrorDisclosable: true);
187+
}
188+
189+
error = TryGetRequiredClaim(jwt, RepositoryOwnerClaim, out _);
190+
if (error != null)
191+
{
192+
return FederatedCredentialPolicyResult.Unauthorized(error, isErrorDisclosable: true);
193+
}
194+
195+
error = TryGetRequiredClaim(jwt, RepositoryOwnerIdClaim, out string repositoryOwnerId);
196+
if (error != null)
197+
{
198+
return FederatedCredentialPolicyResult.Unauthorized(error, isErrorDisclosable: true);
199+
}
200+
201+
error = TryGetRequiredClaim(jwt, RepositoryIdClaim, out string repositoryId);
202+
if (error != null)
203+
{
204+
return FederatedCredentialPolicyResult.Unauthorized(error, isErrorDisclosable: true);
205+
}
206+
207+
error = TryGetRequiredClaim(jwt, EventNameClaim, out string eventName);
208+
if (error != null)
209+
{
210+
return FederatedCredentialPolicyResult.Unauthorized(error, isErrorDisclosable: true);
211+
}
212+
180213
var criteria = GitHubCriteria.FromDatabaseJson(policy.Criteria);
181214

182215
// Validate required GitHub criterias first
183-
string? error = ValidateClaimExactMatch(jwt, RepositoryOwnerClaim, criteria.RepositoryOwner, StringComparison.OrdinalIgnoreCase);
216+
error = ValidateClaimExactMatch(jwt, RepositoryOwnerClaim, criteria.RepositoryOwner, StringComparison.OrdinalIgnoreCase);
184217
if (error != null)
185218
{
186219
return FederatedCredentialPolicyResult.Unauthorized(error);
@@ -203,19 +236,6 @@ public override async Task<FederatedCredentialPolicyResult> EvaluatePolicyAsync(
203236
isErrorDisclosable: true);
204237
}
205238

206-
// Get repo and owner IDs from the token
207-
error = TryGetRequiredClaim(jwt, RepositoryOwnerIdClaim, out string repositoryOwnerId);
208-
if (error != null)
209-
{
210-
return FederatedCredentialPolicyResult.Unauthorized(error);
211-
}
212-
213-
error = TryGetRequiredClaim(jwt, RepositoryIdClaim, out string repositoryId);
214-
if (error != null)
215-
{
216-
return FederatedCredentialPolicyResult.Unauthorized(error);
217-
}
218-
219239
// Update the policy with the repo and owner IDs
220240
criteria.RepositoryOwnerId = repositoryOwnerId;
221241
criteria.RepositoryId = repositoryId;
@@ -266,6 +286,15 @@ public override async Task<FederatedCredentialPolicyResult> EvaluatePolicyAsync(
266286
// IMPORTANT. By now we validated repo owner and repo. Including IDs.
267287
// From now on we can report errors as disclosable.
268288

289+
// Reject banned events
290+
if (_configuration.BannedGitHubActionsEvents is not null
291+
&& _configuration.BannedGitHubActionsEvents.Contains(eventName, StringComparer.OrdinalIgnoreCase))
292+
{
293+
return FederatedCredentialPolicyResult.Unauthorized(
294+
$"The GitHub Actions event '{eventName}' is not allowed.",
295+
isErrorDisclosable: true);
296+
}
297+
269298
// Get workflow ref, e.g. "contoso/contoso-sdk/.github/workflows/release.yml@refs/heads/main"
270299
error = TryGetRequiredClaim(jwt, JobWorkflowRefClaim, out string workflowRef);
271300
if (error != null)

‎tests/NuGet.Services.Configuration.Tests/StringArrayConverterFacts.cs‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
1-
// Copyright (c) .NET Foundation. All rights reserved.
1+
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using Xunit;
55

6+
#nullable enable
7+
68
namespace NuGet.Services.Configuration.Tests
79
{
810
public class StringArrayConverterFacts
@@ -23,15 +25,14 @@ public void SplitsStringBySemicolon()
2325
var output = target.ConvertFrom("foo;bar ; baz");
2426

2527
var array = Assert.IsType<string[]>(output);
26-
Assert.Equal(new[] { "foo", "bar ", " baz" }, array);
28+
Assert.Equal(new[] { "foo", "bar", "baz" }, array);
2729
}
2830

2931
[Theory]
3032
[InlineData("foo")]
3133
[InlineData("foo bar")]
3234
[InlineData("foo|bar")]
3335
[InlineData("foo,bar")]
34-
[InlineData(" ")]
3536
public void ReturnsSingleStringWhenThereIsNoDelimeter(string input)
3637
{
3738
var target = new StringArrayConverter();
@@ -42,12 +43,15 @@ public void ReturnsSingleStringWhenThereIsNoDelimeter(string input)
4243
Assert.Equal(new[] { input }, array);
4344
}
4445

45-
[Fact]
46-
public void ReturnsEmptyArrayWithEmpty()
46+
[Theory]
47+
[InlineData(null)]
48+
[InlineData("")]
49+
[InlineData(" ")]
50+
public void ReturnsEmptyArrayWithEmpty(string? input)
4751
{
4852
var target = new StringArrayConverter();
4953

50-
var output = target.ConvertFrom(string.Empty);
54+
var output = target.ConvertFrom(input);
5155

5256
var array = Assert.IsType<string[]>(output);
5357
Assert.Empty(array);

‎tests/NuGetGallery.Facts/Authentication/Federated/GitHubTokenPolicyValidatorFacts.cs‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,8 @@ public class TokenTestHelper
3030
{ "repository_id", "id-456" },
3131
{ "job_workflow_ref", "test-owner/test-repo/.github/workflows/test.yml@refs/heads/main" },
3232
{ "environment", "production" },
33-
{ "jti", "test-token-id" }
33+
{ "jti", "test-token-id" },
34+
{ "event_name", "release" },
3435
};
3536

3637
public const string PermanentPolicyCriteria = """
@@ -589,6 +590,7 @@ public async Task RejectsExpiredTemporaryPolicy()
589590
[InlineData("repository_owner_id")]
590591
[InlineData("repository")]
591592
[InlineData("repository_id")]
593+
[InlineData("event_name")]
592594
public async Task RejectsMissingRequiredClaim(string claim)
593595
{
594596
// Arrange
@@ -609,13 +611,37 @@ public async Task RejectsMissingRequiredClaim(string claim)
609611

610612
// Assert
611613
Assert.Equal(FederatedCredentialPolicyResultType.Unauthorized, resultEmpty.Type);
612-
Assert.False(resultEmpty.IsErrorDisclosable);
614+
Assert.True(resultEmpty.IsErrorDisclosable);
613615
Assert.Contains(claim, resultEmpty.Error);
614616
Assert.Equal(FederatedCredentialPolicyResultType.Unauthorized, resultMissing.Type);
615-
Assert.False(resultMissing.IsErrorDisclosable);
617+
Assert.True(resultMissing.IsErrorDisclosable);
616618
Assert.Contains(claim, resultMissing.Error);
617619
}
618620

621+
[Fact]
622+
public async Task RejectsBannedEventName()
623+
{
624+
// Arrange
625+
var createdBy = new User("test-user");
626+
var policy = new FederatedCredentialPolicy
627+
{
628+
Type = FederatedCredentialType.GitHubActions,
629+
Criteria = TokenTestHelper.PermanentPolicyCriteria,
630+
CreatedBy = createdBy
631+
};
632+
Configuration.Setup(c => c.BannedGitHubActionsEvents).Returns(["bad_one"]);
633+
634+
var tokenWithBannedValue = TokenTestHelper.CreateTestJwtWithCustomClaimValue("event_name", "bad_one");
635+
636+
// Act
637+
var result = await Target.EvaluatePolicyAsync(policy, tokenWithBannedValue);
638+
639+
// Assert
640+
Assert.Equal(FederatedCredentialPolicyResultType.Unauthorized, result.Type);
641+
Assert.True(result.IsErrorDisclosable);
642+
Assert.Contains("bad_one", result.Error);
643+
}
644+
619645
[Theory]
620646
[InlineData("repository_owner", false)]
621647
[InlineData("repository_owner_id", false)]

0 commit comments

Comments
 (0)