From f36ddc7532553265b79b44c3d9e0cb2f07249492 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 16 Nov 2022 11:52:58 -0800 Subject: [PATCH 1/5] bug fixes related to the unmanaged nodes - fix the token request in the client - adding a is_unmnaged field to the agentConfig - fix typo in the appId claim type - fix typo in the UnmanagedNode claim value - fix query PoolOperation.GetByClientId --- src/ApiService/ApiService/Functions/Pool.cs | 3 ++- src/ApiService/ApiService/OneFuzzTypes/Model.cs | 5 ++--- src/ApiService/ApiService/UserCredentials.cs | 3 +-- .../onefuzzlib/EndpointAuthorization.cs | 13 +++++++------ .../ApiService/onefuzzlib/PoolOperations.cs | 2 +- src/agent/onefuzz-agent/src/config.rs | 16 +++++++++++++--- src/agent/onefuzz-agent/src/main.rs | 6 +++--- src/agent/onefuzz/src/auth.rs | 1 - src/cli/onefuzz/api.py | 8 +------- src/pytypes/onefuzztypes/models.py | 1 + 10 files changed, 31 insertions(+), 27 deletions(-) diff --git a/src/ApiService/ApiService/Functions/Pool.cs b/src/ApiService/ApiService/Functions/Pool.cs index d9fefc49fe..830f750a00 100644 --- a/src/ApiService/ApiService/Functions/Pool.cs +++ b/src/ApiService/ApiService/Functions/Pool.cs @@ -133,7 +133,8 @@ private async Task Populate(PoolGetResult p, bool skipSummaries = HeartbeatQueue: queueSas, InstanceId: instanceId, ClientCredentials: null, - MultiTenantDomain: _context.ServiceConfiguration.MultiTenantDomain) + MultiTenantDomain: _context.ServiceConfiguration.MultiTenantDomain, + IsUnmanaged: !p.Managed) }; } } diff --git a/src/ApiService/ApiService/OneFuzzTypes/Model.cs b/src/ApiService/ApiService/OneFuzzTypes/Model.cs index c7620ea38c..9e7411b1c0 100644 --- a/src/ApiService/ApiService/OneFuzzTypes/Model.cs +++ b/src/ApiService/ApiService/OneFuzzTypes/Model.cs @@ -678,12 +678,11 @@ public record AgentConfig( string? InstanceTelemetryKey, string? MicrosoftTelemetryKey, string? MultiTenantDomain, - Guid InstanceId + Guid InstanceId, + bool? IsUnmanaged ); - - public record Vm( string Name, Region Region, diff --git a/src/ApiService/ApiService/UserCredentials.cs b/src/ApiService/ApiService/UserCredentials.cs index 7446b0fd9b..fb9bf2d7f7 100644 --- a/src/ApiService/ApiService/UserCredentials.cs +++ b/src/ApiService/ApiService/UserCredentials.cs @@ -77,7 +77,7 @@ public virtual async Task> ParseJwtToken(HttpRequest switch (claim.Type) { case "oid": return acc with { UserInfo = acc.UserInfo with { ObjectId = Guid.Parse(claim.Value) } }; - case "appId": + case "appid": return acc with { UserInfo = acc.UserInfo with { ApplicationId = Guid.Parse(claim.Value) } }; case "upn": return acc with { UserInfo = acc.UserInfo with { Upn = claim.Value } }; @@ -88,7 +88,6 @@ public virtual async Task> ParseJwtToken(HttpRequest return acc; } }); - return OneFuzzResult.Ok(userInfo); } else { var tenantsStr = allowedTenants.OkV is null ? "null" : String.Join(';', allowedTenants.OkV!); diff --git a/src/ApiService/ApiService/onefuzzlib/EndpointAuthorization.cs b/src/ApiService/ApiService/onefuzzlib/EndpointAuthorization.cs index 045d43ac9e..6df6eaa51e 100644 --- a/src/ApiService/ApiService/onefuzzlib/EndpointAuthorization.cs +++ b/src/ApiService/ApiService/onefuzzlib/EndpointAuthorization.cs @@ -30,8 +30,7 @@ public class EndpointAuthorization : IEndpointAuthorization { private readonly IOnefuzzContext _context; private readonly ILogTracer _log; private readonly GraphServiceClient _graphClient; - - private static readonly HashSet AgentRoles = new HashSet { "UnmamagedNode", "ManagedNode" }; + private static readonly HashSet AgentRoles = new HashSet { "UnmanagedNode", "ManagedNode" }; public EndpointAuthorization(IOnefuzzContext context, ILogTracer log, GraphServiceClient graphClient) { _context = context; @@ -46,8 +45,8 @@ public virtual async Async.Task CallIf(HttpRequestData req, Fu return await _context.RequestHandling.NotOk(req, tokenResult.ErrorV, "token verification", HttpStatusCode.Unauthorized); } - var token = tokenResult.OkV; - if (await IsUser(token)) { + var token = tokenResult.OkV.UserInfo; + if (await IsUser(tokenResult.OkV)) { if (!allowUser) { return await Reject(req, tokenResult.OkV.UserInfo); } @@ -58,7 +57,7 @@ public virtual async Async.Task CallIf(HttpRequestData req, Fu } } - if (await IsAgent(token) && !allowAgent) { + if (await IsAgent(tokenResult.OkV) && !allowAgent) { return await Reject(req, tokenResult.OkV.UserInfo); } @@ -201,7 +200,9 @@ public async Async.Task IsAgent(UserAuthInfo authInfo) { } var principalId = await _context.Creds.GetScalesetPrincipalId(); - return principalId == tokenData.ObjectId; + if (principalId == tokenData.ObjectId) { + return true; + } } if (!tokenData.ApplicationId.HasValue) { diff --git a/src/ApiService/ApiService/onefuzzlib/PoolOperations.cs b/src/ApiService/ApiService/onefuzzlib/PoolOperations.cs index 27867ab470..9170a240b5 100644 --- a/src/ApiService/ApiService/onefuzzlib/PoolOperations.cs +++ b/src/ApiService/ApiService/onefuzzlib/PoolOperations.cs @@ -89,7 +89,7 @@ public async Task ScheduleWorkset(Pool pool, WorkSet workSet) { } public IAsyncEnumerable GetByClientId(Guid clientId) { - return QueryAsync(filter: TableClient.CreateQueryFilter($"client_id eq {clientId}")); + return QueryAsync(filter: $"client_id eq '{clientId}'"); } public string GetPoolQueue(Guid poolId) diff --git a/src/agent/onefuzz-agent/src/config.rs b/src/agent/onefuzz-agent/src/config.rs index 7d3060e2e5..3a7b5fadf9 100644 --- a/src/agent/onefuzz-agent/src/config.rs +++ b/src/agent/onefuzz-agent/src/config.rs @@ -34,12 +34,15 @@ pub struct StaticConfig { pub heartbeat_queue: Option, pub instance_id: Uuid, + + #[serde(default)] + pub is_unmanaged: bool, } // Temporary shim type to bridge the current service-provided config. #[derive(Clone, Debug, Deserialize, Eq, PartialEq)] struct RawStaticConfig { - pub credentials: Option, + pub client_credentials: Option, pub pool_name: String, @@ -54,13 +57,16 @@ struct RawStaticConfig { pub heartbeat_queue: Option, pub instance_id: Uuid, + + #[serde(default)] + pub is_unmanaged: bool, } impl StaticConfig { pub fn new(data: &[u8]) -> Result { let config: RawStaticConfig = serde_json::from_slice(data)?; - let credentials = match config.credentials { + let credentials = match config.client_credentials { Some(client) => client.into(), None => { // Remove trailing `/`, which is treated as a distinct resource. @@ -83,6 +89,7 @@ impl StaticConfig { instance_telemetry_key: config.instance_telemetry_key, heartbeat_queue: config.heartbeat_queue, instance_id: config.instance_id, + is_unmanaged: config.is_unmanaged, }; Ok(config) @@ -103,6 +110,7 @@ impl StaticConfig { let multi_tenant_domain = std::env::var("ONEFUZZ_MULTI_TENANT_DOMAIN").ok(); let onefuzz_url = Url::parse(&std::env::var("ONEFUZZ_URL")?)?; let pool_name = std::env::var("ONEFUZZ_POOL")?; + let is_unmanaged = std::env::var("ONEFUZZ_IS_UNMANAGED").is_ok(); let heartbeat_queue = if let Ok(key) = std::env::var("ONEFUZZ_HEARTBEAT") { Some(Url::parse(&key)?) @@ -142,6 +150,7 @@ impl StaticConfig { microsoft_telemetry_key, heartbeat_queue, instance_id, + is_unmanaged, }) } @@ -213,7 +222,8 @@ impl Registration { .append_pair("machine_id", &machine_id.to_string()) .append_pair("machine_name", &machine_name) .append_pair("pool_name", &config.pool_name) - .append_pair("version", env!("ONEFUZZ_VERSION")); + .append_pair("version", env!("ONEFUZZ_VERSION")) + .append_pair("os", std::env::consts::OS); if managed { let scaleset = onefuzz::machine_id::get_scaleset_name().await?; diff --git a/src/agent/onefuzz-agent/src/main.rs b/src/agent/onefuzz-agent/src/main.rs index 56ff42c7b1..14a8852e78 100644 --- a/src/agent/onefuzz-agent/src/main.rs +++ b/src/agent/onefuzz-agent/src/main.rs @@ -277,10 +277,10 @@ async fn run_agent(config: StaticConfig) -> Result<()> { let registration = match config::Registration::load_existing(config.clone()).await { Ok(registration) => registration, Err(_) => { - if scaleset.is_some() { - config::Registration::create_managed(config.clone()).await? - } else { + if config.is_unmanaged { config::Registration::create_unmanaged(config.clone()).await? + } else { + config::Registration::create_managed(config.clone()).await? } } }; diff --git a/src/agent/onefuzz/src/auth.rs b/src/agent/onefuzz/src/auth.rs index 19a9b08d66..d25a3807f9 100644 --- a/src/agent/onefuzz/src/auth.rs +++ b/src/agent/onefuzz/src/auth.rs @@ -134,7 +134,6 @@ impl ClientCredentials { let response = reqwest::Client::new() .post(url) - .header("Content-Length", "0") .form(&[ ("client_id", self.client_id.to_hyphenated().to_string()), ("client_secret", self.client_secret.expose_ref().to_string()), diff --git a/src/cli/onefuzz/api.py b/src/cli/onefuzz/api.py index b091ded41e..c77e4e3dd6 100644 --- a/src/cli/onefuzz/api.py +++ b/src/cli/onefuzz/api.py @@ -1268,13 +1268,7 @@ def get_config(self, pool_name: primitives.PoolName) -> models.AgentConfig: if pool.config is None: raise Exception("Missing AgentConfig in response") - config = pool.config - config.client_credentials = models.ClientCredentials( # nosec - bandit consider this a hard coded password - client_id=pool.client_id, - client_secret="", - ) - - return config + return pool.config def shutdown(self, name: str, *, now: bool = False) -> responses.BoolResult: expanded_name = self._disambiguate( diff --git a/src/pytypes/onefuzztypes/models.py b/src/pytypes/onefuzztypes/models.py index 0913fcbf61..cd00f71678 100644 --- a/src/pytypes/onefuzztypes/models.py +++ b/src/pytypes/onefuzztypes/models.py @@ -339,6 +339,7 @@ class AgentConfig(BaseModel): microsoft_telemetry_key: Optional[str] multi_tenant_domain: Optional[str] instance_id: UUID + is_unmanaged: Optional[bool] class TaskUnitConfig(BaseModel): From 56fc81237b85bf3762392119d55f56ca579da41d Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 16 Nov 2022 12:17:18 -0800 Subject: [PATCH 2/5] remove unused import --- src/ApiService/ApiService/onefuzzlib/Extension.cs | 3 ++- src/ApiService/ApiService/onefuzzlib/PoolOperations.cs | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ApiService/ApiService/onefuzzlib/Extension.cs b/src/ApiService/ApiService/onefuzzlib/Extension.cs index d9237104bd..dfdace7b93 100644 --- a/src/ApiService/ApiService/onefuzzlib/Extension.cs +++ b/src/ApiService/ApiService/onefuzzlib/Extension.cs @@ -220,7 +220,8 @@ public static VMExtensionWrapper GenevaExtension(AzureLocation region) { InstanceTelemetryKey: _context.ServiceConfiguration.ApplicationInsightsInstrumentationKey, MicrosoftTelemetryKey: _context.ServiceConfiguration.OneFuzzTelemetry, MultiTenantDomain: _context.ServiceConfiguration.MultiTenantDomain, - InstanceId: instanceId + InstanceId: instanceId, + IsUnmanaged: !pool.Managed ); var fileName = $"{pool.Name}/config.json"; diff --git a/src/ApiService/ApiService/onefuzzlib/PoolOperations.cs b/src/ApiService/ApiService/onefuzzlib/PoolOperations.cs index 9170a240b5..09c0ec342c 100644 --- a/src/ApiService/ApiService/onefuzzlib/PoolOperations.cs +++ b/src/ApiService/ApiService/onefuzzlib/PoolOperations.cs @@ -1,6 +1,5 @@ using System.Threading.Tasks; using ApiService.OneFuzzLib.Orm; -using Azure.Data.Tables; namespace Microsoft.OneFuzz.Service; public interface IPoolOperations : IStatefulOrm { From 19b3a3fc63f26cff80ea7b5bac0d7de574820a80 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 16 Nov 2022 13:00:41 -0800 Subject: [PATCH 3/5] build fix --- src/proxy-manager/src/proxy.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/proxy-manager/src/proxy.rs b/src/proxy-manager/src/proxy.rs index f270cddd30..d0a1fbc36c 100644 --- a/src/proxy-manager/src/proxy.rs +++ b/src/proxy-manager/src/proxy.rs @@ -90,7 +90,7 @@ pub async fn update(data: &ConfigData) -> Result<()> { } let file_name = path.file_name().unwrap().to_string_lossy().to_string(); - if !file_name.starts_with(&PROXY_PREFIX) { + if !file_name.starts_with(PROXY_PREFIX) { continue; } From ea878f517c1617bc6addf32d22a56d179f3b5fe8 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 17 Nov 2022 11:01:24 -0800 Subject: [PATCH 4/5] change unmanaged field to managed --- src/ApiService/ApiService/Functions/Pool.cs | 2 +- src/ApiService/ApiService/OneFuzzTypes/Model.cs | 2 +- .../ApiService/onefuzzlib/Extension.cs | 2 +- src/agent/onefuzz-agent/src/config.rs | 16 ++++++++++------ src/agent/onefuzz-agent/src/main.rs | 6 +++--- src/pytypes/onefuzztypes/models.py | 2 +- 6 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/ApiService/ApiService/Functions/Pool.cs b/src/ApiService/ApiService/Functions/Pool.cs index 830f750a00..6ca19ec640 100644 --- a/src/ApiService/ApiService/Functions/Pool.cs +++ b/src/ApiService/ApiService/Functions/Pool.cs @@ -134,7 +134,7 @@ private async Task Populate(PoolGetResult p, bool skipSummaries = InstanceId: instanceId, ClientCredentials: null, MultiTenantDomain: _context.ServiceConfiguration.MultiTenantDomain, - IsUnmanaged: !p.Managed) + Managed: p.Managed) }; } } diff --git a/src/ApiService/ApiService/OneFuzzTypes/Model.cs b/src/ApiService/ApiService/OneFuzzTypes/Model.cs index 9e7411b1c0..09a1bc4e90 100644 --- a/src/ApiService/ApiService/OneFuzzTypes/Model.cs +++ b/src/ApiService/ApiService/OneFuzzTypes/Model.cs @@ -679,7 +679,7 @@ public record AgentConfig( string? MicrosoftTelemetryKey, string? MultiTenantDomain, Guid InstanceId, - bool? IsUnmanaged + bool? Managed = true ); diff --git a/src/ApiService/ApiService/onefuzzlib/Extension.cs b/src/ApiService/ApiService/onefuzzlib/Extension.cs index dfdace7b93..a9bc508852 100644 --- a/src/ApiService/ApiService/onefuzzlib/Extension.cs +++ b/src/ApiService/ApiService/onefuzzlib/Extension.cs @@ -221,7 +221,7 @@ public static VMExtensionWrapper GenevaExtension(AzureLocation region) { MicrosoftTelemetryKey: _context.ServiceConfiguration.OneFuzzTelemetry, MultiTenantDomain: _context.ServiceConfiguration.MultiTenantDomain, InstanceId: instanceId, - IsUnmanaged: !pool.Managed + Managed: pool.Managed ); var fileName = $"{pool.Name}/config.json"; diff --git a/src/agent/onefuzz-agent/src/config.rs b/src/agent/onefuzz-agent/src/config.rs index 3a7b5fadf9..e0fafe4841 100644 --- a/src/agent/onefuzz-agent/src/config.rs +++ b/src/agent/onefuzz-agent/src/config.rs @@ -35,8 +35,12 @@ pub struct StaticConfig { pub instance_id: Uuid, - #[serde(default)] - pub is_unmanaged: bool, + #[serde(default = "default_as_true")] + pub managed: bool, +} + +fn default_as_true() -> bool { + true } // Temporary shim type to bridge the current service-provided config. @@ -58,8 +62,8 @@ struct RawStaticConfig { pub instance_id: Uuid, - #[serde(default)] - pub is_unmanaged: bool, + #[serde(default = "default_as_true")] + pub managed: bool, } impl StaticConfig { @@ -89,7 +93,7 @@ impl StaticConfig { instance_telemetry_key: config.instance_telemetry_key, heartbeat_queue: config.heartbeat_queue, instance_id: config.instance_id, - is_unmanaged: config.is_unmanaged, + managed: config.managed, }; Ok(config) @@ -150,7 +154,7 @@ impl StaticConfig { microsoft_telemetry_key, heartbeat_queue, instance_id, - is_unmanaged, + managed: !is_unmanaged, }) } diff --git a/src/agent/onefuzz-agent/src/main.rs b/src/agent/onefuzz-agent/src/main.rs index 14a8852e78..642e453c98 100644 --- a/src/agent/onefuzz-agent/src/main.rs +++ b/src/agent/onefuzz-agent/src/main.rs @@ -277,10 +277,10 @@ async fn run_agent(config: StaticConfig) -> Result<()> { let registration = match config::Registration::load_existing(config.clone()).await { Ok(registration) => registration, Err(_) => { - if config.is_unmanaged { - config::Registration::create_unmanaged(config.clone()).await? - } else { + if config.managed { config::Registration::create_managed(config.clone()).await? + } else { + config::Registration::create_unmanaged(config.clone()).await? } } }; diff --git a/src/pytypes/onefuzztypes/models.py b/src/pytypes/onefuzztypes/models.py index cd00f71678..858dd18c3d 100644 --- a/src/pytypes/onefuzztypes/models.py +++ b/src/pytypes/onefuzztypes/models.py @@ -339,7 +339,7 @@ class AgentConfig(BaseModel): microsoft_telemetry_key: Optional[str] multi_tenant_domain: Optional[str] instance_id: UUID - is_unmanaged: Optional[bool] + managed: Optional[bool] = Field(default=True) class TaskUnitConfig(BaseModel): From c4cc23624123b0fb0f5ee149bb8c1b0c5435a44b Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 17 Nov 2022 11:03:10 -0800 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Teo Voinea <58236992+tevoinea@users.noreply.github.com> --- src/ApiService/ApiService/onefuzzlib/EndpointAuthorization.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ApiService/ApiService/onefuzzlib/EndpointAuthorization.cs b/src/ApiService/ApiService/onefuzzlib/EndpointAuthorization.cs index 6df6eaa51e..6eccef84a4 100644 --- a/src/ApiService/ApiService/onefuzzlib/EndpointAuthorization.cs +++ b/src/ApiService/ApiService/onefuzzlib/EndpointAuthorization.cs @@ -48,7 +48,7 @@ public virtual async Async.Task CallIf(HttpRequestData req, Fu var token = tokenResult.OkV.UserInfo; if (await IsUser(tokenResult.OkV)) { if (!allowUser) { - return await Reject(req, tokenResult.OkV.UserInfo); + return await Reject(req, token); } var access = await CheckAccess(req); @@ -58,7 +58,7 @@ public virtual async Async.Task CallIf(HttpRequestData req, Fu } if (await IsAgent(tokenResult.OkV) && !allowAgent) { - return await Reject(req, tokenResult.OkV.UserInfo); + return await Reject(req, token); } return await method(req);