This repository has been archived by the owner on Nov 1, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added support for multi-tenant authentication #563
Added support for multi-tenant authentication #563
Changes from all commits
2443280
93c3fcd
669e728
3f53c1c
e2e8569
8356243
77ba272
3e79876
e7021a1
f8fda80
ac95cd3
72f3d6e
c2d1793
11ccd1d
2a14b57
aadb0fe
8233e66
2d90809
aa15f10
fefc9d4
a1acf00
987e876
69f98b2
ec9ede0
210f167
ff74d1a
14f3108
fdf1ed0
c45630e
7dfad83
9595b6b
ffe118a
3553a08
6874719
d88fbdb
a5aa3f4
499f50d
3a9067a
7cbe7a4
a1dc8c9
0578941
0b329cb
bc15097
d3deb9a
93f16b9
cd19cdf
262cecf
7f5eebe
79c42c3
a6b4625
ab3b65b
25d6827
4ae1ddc
5c9fcb0
8944153
db98c73
3fd1b2b
7c9c0d1
3a6b0ed
14d5cd6
c408410
c21ec1d
7d48e77
194fbe0
c1f0b61
c612e20
7e740d6
f9d1eca
387a9a3
45307b2
687ea3d
2e98181
b50ad94
6420970
13e515c
1a63ca0
9c6bc01
22497d2
9264f4e
fdb37a4
04e88db
552da77
1e1c525
9743c87
ec0085c
d0d8f68
59be78d
b823b74
5e0bd10
1075624
f6f13a0
4562ae8
5c1e7ae
e02c1f6
be548fc
365b35e
3c9dc6f
bffadd9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a suggestion, as pointed out before, this
unwrap
may panic if it's an invalid url. Since the return type is notResult<>
, an alternative could be using expect, which will still panic but allows you to pass a string error message.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a better alternative, but still panics, which we want to avoid.
I suggest doing some validation earlier. For example, you could validate and precompute
multi_tenant_domain
in the ctor (ManagedIdentityCredentials::new()
), which would then return aResult
.@anslutsk, using some stand-in values, could you share what we expect
resource
and a validmanaged_identity_url
to look like?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put in some temporary println statements to provide in context here.
In the case where OneFuzz is deployed in single tenant mode, the config.json file contains an entry for "multi_tenant_config" which has value "null" and the self.resource String value is "https://anslutsk-26.azurewebsites.net" and thus, the pattern "if let Some(domain)" is not triggered, so the else clause is run, which executes the line: self.resource.clone().
In the case where OneFuzz is deployed in multi tenant mode, the config.json file contains the entry "multi_tenant_config" with the value "mspmecloud.onmicrosoft.com" and the self.resource String value is "https://anslutsk-26.azurewebsites.net" and so the pattern "if let Some(domain)" is triggered, so the corresponding code block is executed and the value of self.instance[0] is "anslutsk-26"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When there was an invalid domain in the "multi_tenant_config" json config item, for example the String value "...." the program did not crash and we get the expected error during while trying to obtain a Bearer token from IMS:
[2021-02-24T07:12:19Z ERROR onefuzz_telemetry] error running supervisor agent: 400 Bad Request: {"error":"invalid_resource","error_description":"AADSTS500011: The resource principal named https://..../anslutsk-multi-26 was not found in the tenant named 975f013f-7f24-47e8-a7d3-abc4752bf346. This can happen if the application has not been installed by the administrator of the tenant or consented to by any user in the tenant. You might have sent your authentication request to the wrong tenant.\r\nTrace ID: 0eec3338-bfa4-49f1-a7aa-7d13f9842200\r\nCorrelation ID: 02d18935-c309-4c71-8757-7a9770a98805\r\nTimestamp: 2021-02-24 07:12:19Z","error_codes":[500011],"timestamp":"2021-02-24 07:12:19Z","trace_id":"0eec3338-bfa4-49f1-a7aa-7d13f9842200","correlation_id":"02d18935-c309-4c71-8757-7a9770a98805","error_uri":"https://westus2.login.microsoft.com/error?code=500011"}
In addition, I ran an example of another "error" case:
When there was an invalid domain in the "multi_tenant_config" json config item, for example the String value "asdfasdfasdf" the program did not crash and we get the expected error during while trying to obtain a Bearer token from IMS:
[2021-02-24T07:16:33Z ERROR onefuzz_telemetry] error running supervisor agent: 400 Bad Request: {"error":"invalid_resource","error_description":"AADSTS500011: The resource principal named https://asdfasdfasdf/anslutsk-multi-26 was not found in the tenant named 975f013f-7f24-47e8-a7d3-abc4752bf346. This can happen if the application has not been installed by the administrator of the tenant or consented to by any user in the tenant. You might have sent your authentication request to the wrong tenant.\r\nTrace ID: b0c8b83c-e552-4f2f-9f77-d280785d2600\r\nCorrelation ID: 1982fe67-c50a-4e0a-ac63-186134839aae\r\nTimestamp: 2021-02-24 07:16:33Z","error_codes":[500011],"timestamp":"2021-02-24 07:16:33Z","trace_id":"b0c8b83c-e552-4f2f-9f77-d280785d2600","correlation_id":"1982fe67-c50a-4e0a-ac63-186134839aae","error_uri":"https://westus2.login.microsoft.com/error?code=500011"}
I do not see the point in adding additional error handling. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value assigned to self.resource is taken from the config item "onefuzz_url" and if the value is invalid, the program crashes like this:
[2021-02-24T15:54:35Z ERROR onefuzz_telemetry] error loading supervisor agent config: invalid value: string "asdf", expected description() is deprecated; use Display at line 1 column 50
If the "onefuzz_url" is set to "null" the following error is displayed:
[2021-02-24T16:03:23Z ERROR onefuzz_telemetry] error loading supervisor agent config: invalid type: null, expected a string representing an URL at line 1 column 48
Error: invalid type: null, expected a string representing an URL at line 1 column 48
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessary to change this section of the code, but that doesn't mean that I don't support making a change! If either of you would like to suggest a specific change then I'd be happy to test it. It would need to be pretty specific though since I don't share either of your view points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing examples of expected/valid inputs, and your investigation into the current failure modes.
We still want to change what we're doing here, because this de facto current unreachability is very non-local, and depends on the behavior of the larger system. But if we ever accidentally introduce a bug elsewhere that makes the
unwrap()
reachable, we want to fail in a way that will be easier to understand and fix, and get reported here.So, the constraint here is that we must not
unwrap()
. We could makeManagedIdentityCredentials::url()
return aResult<Url>
, but that would be a bit silly, since we'd then have to change all use sites, and a non-Ok()
result is actually an error that we can detect much earlier (in the constructor).Instead, I suggest changing
ManagedIdentityCredentials::new()
to have the signaturenew(Url, Option<Url>) -> Result<Self>
, taking care to generate theresource
string without a trailing/
in the non-multi-tenant case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test123