-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
add RP API More Secure Defaults Guidance #566
base: vNext
Are you sure you want to change the base?
add RP API More Secure Defaults Guidance #566
Conversation
@Stefan0Anghel please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
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.
Provided minor comments.
@@ -1292,6 +1292,52 @@ In addition to distributed tracing, Azure also uses a set of common correlation | |||
|x-ms-client-request-id |Both |Optional. Caller-specified value identifying the request, in the form of a GUID with no decoration such as curly braces (e.g. `x-ms-client-request-id: 9C4D50EE-2D56-4CD3-8152-34347DC9F2B0`). If the caller provides this header the service **must** include this in their log entries to facilitate correlation of log entries for a single request. Because this header can be client-generated, it should not be assumed to be unique by the service implementation. | |||
|x-ms-request-id |Response |Required. Service generated correlation id identifying the request, in the form of a GUID with no decoration such as curly braces. In contrast to the the `x-ms-client-request-id`, the service **must** ensure that this value is globally unique. Services should log this value with their traces to facilitate correlation of log entries for a single request. | |||
|
|||
<a href="#RP-API-MSD" name="RP-API-MSD"></a> |
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.
Rename to a more presentable display name instead of RP-API-MSD?
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.
Why do RP-specific guidelines go in the data-plane guidelines?
Maybe we want to adopt this for data-plane; I haven't reviewed this PR enough to know.
@@ -1292,6 +1292,52 @@ In addition to distributed tracing, Azure also uses a set of common correlation | |||
|x-ms-client-request-id |Both |Optional. Caller-specified value identifying the request, in the form of a GUID with no decoration such as curly braces (e.g. `x-ms-client-request-id: 9C4D50EE-2D56-4CD3-8152-34347DC9F2B0`). If the caller provides this header the service **must** include this in their log entries to facilitate correlation of log entries for a single request. Because this header can be client-generated, it should not be assumed to be unique by the service implementation. | |||
|x-ms-request-id |Response |Required. Service generated correlation id identifying the request, in the form of a GUID with no decoration such as curly braces. In contrast to the the `x-ms-client-request-id`, the service **must** ensure that this value is globally unique. Services should log this value with their traces to facilitate correlation of log entries for a single request. | |||
|
|||
<a href="#RP-API-MSD" name="RP-API-MSD"></a> | |||
### RP API More Secure Defaults Guidance |
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.
RP -> ARM Resource Provider?
Something like: "### ARM Resource Provider More Secure Default Parameter Guidance"
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.
This is not the right document for ARM API guidelines.
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.
True, this document is not the right place for arm/rp guidelines. However, I think adding some of these guidelines to this document for data plane services is perfectly okay
<a href="#RP-API-MSD" name="RP-API-MSD"></a> | ||
### RP API More Secure Defaults Guidance | ||
|
||
The RP API MSD Guidelines are designed to ensure that any new RP API being developed is deployed from the outset with secure parameter default values. This proactive approach helps prevent non-secure defaults and maintains a high security standard from the very beginning of the API development process. |
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.
...either more secure default parameter values or making the previously optional parameter as required"
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.
Defaulting vs. explicitness is a difficult trade-off.
Default values allow the service to improve its security posture over time with new capabilities without forcing clients to change. Having clients explicitly "pin" values can slow down adoption of new capabilities. Adding new required parameters means breaking changes. Which slows down adoption of new api versions. Which in turn slows down the ability to use the new features.
In practical terms, we need to have a default value for new security related properties when we migrate previously existing resources. That doesn't have to mean that we use the same default values in the public api definitions, but it doesn't seem like an unreasonable thing to do either.
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 have had numerous conversations about services exposing default values for properties and have been meaning to write a guideline about it. The guideline would go something like this: when a structure/model is first introduced by a service (V1 of structure), very few (if any) of the properties should have default values. Customer must explicitly select values for the properties. This prevents breaking changes should the service want to change a default since there are no defaults. Any new properties that are added to the structure/model in the future (V2+) must have default values in order to avoid breaking changes.
The RP API MSD Guidelines are designed to ensure that any new RP API being developed is deployed from the outset with secure parameter default values. This proactive approach helps prevent non-secure defaults and maintains a high security standard from the very beginning of the API development process. | ||
|
||
<a href="#rp-parameter-defining" name="rp-parameter-defining">:white_check_mark:</a> **DO** define parameters as explicit as possible: | ||
- Tagging parameters as required or not |
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.
...as "required", especially if security-sensitive.
|
||
<a href="#rp-parameter-defining" name="rp-parameter-defining">:white_check_mark:</a> **DO** define parameters as explicit as possible: | ||
- Tagging parameters as required or not | ||
- If a parameter is not tagged as required and it has security implications, define a default value |
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.
...define a default value which is deemed as more secure
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 would say that there just shouldn't be a default and that the property is required - period.
<a href="#rp-parameter-defining" name="rp-parameter-defining">:white_check_mark:</a> **DO** define parameters as explicit as possible: | ||
- Tagging parameters as required or not | ||
- If a parameter is not tagged as required and it has security implications, define a default value | ||
- Defining the default value as a `key: "value"` pair |
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.
- Providing a concise but descriptive description of the parameters in the API
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 just want to be clear that changing a default value is a breaking change and requires approval from the breaking change review board. This can all be avoided by not having defaults and instead have customers explicitly select what they desire.
@@ -1292,6 +1292,52 @@ In addition to distributed tracing, Azure also uses a set of common correlation | |||
|x-ms-client-request-id |Both |Optional. Caller-specified value identifying the request, in the form of a GUID with no decoration such as curly braces (e.g. `x-ms-client-request-id: 9C4D50EE-2D56-4CD3-8152-34347DC9F2B0`). If the caller provides this header the service **must** include this in their log entries to facilitate correlation of log entries for a single request. Because this header can be client-generated, it should not be assumed to be unique by the service implementation. |
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.
To expand on this (and why it is important for services to not make assumptions about this value) - client request id is often used to group a set of "logical" client requests. Retries will use the same client request id. I believe that tools like the azure cli may use the same client request id for each request made in a single command execution.
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 agree with Johan here. I consider this header and it's value purely for client benefit. I don't think it has to be a guid. I think a service should specify a max length and perhaps valid characters. But I don't think more than this is required. Also, what is motivating that a service MUST log this value?
], | ||
``` | ||
|
||
<a href="#security-parameters-handling" name="security-parameters-handling">:ballot_box_with_check:</a> **YOU SHOULD** consider tagging a security parameter as required. If making it required is not feasible, refer to the Microsoft Cloud Security Benchmark in order to assign a meaningful value to avoid security gaps. If you need to deviate from this default value, discuss matter within your team and with the API Board to ensure awareness and understanding of the parameter. |
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.
What is a "security parameter"?
@@ -1292,6 +1292,52 @@ In addition to distributed tracing, Azure also uses a set of common correlation | |||
|x-ms-client-request-id |Both |Optional. Caller-specified value identifying the request, in the form of a GUID with no decoration such as curly braces (e.g. `x-ms-client-request-id: 9C4D50EE-2D56-4CD3-8152-34347DC9F2B0`). If the caller provides this header the service **must** include this in their log entries to facilitate correlation of log entries for a single request. Because this header can be client-generated, it should not be assumed to be unique by the service implementation. |
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 agree with Johan here. I consider this header and it's value purely for client benefit. I don't think it has to be a guid. I think a service should specify a max length and perhaps valid characters. But I don't think more than this is required. Also, what is motivating that a service MUST log this value?
@@ -1292,6 +1292,52 @@ In addition to distributed tracing, Azure also uses a set of common correlation | |||
|x-ms-client-request-id |Both |Optional. Caller-specified value identifying the request, in the form of a GUID with no decoration such as curly braces (e.g. `x-ms-client-request-id: 9C4D50EE-2D56-4CD3-8152-34347DC9F2B0`). If the caller provides this header the service **must** include this in their log entries to facilitate correlation of log entries for a single request. Because this header can be client-generated, it should not be assumed to be unique by the service implementation. | |||
|x-ms-request-id |Response |Required. Service generated correlation id identifying the request, in the form of a GUID with no decoration such as curly braces. In contrast to the the `x-ms-client-request-id`, the service **must** ensure that this value is globally unique. Services should log this value with their traces to facilitate correlation of log entries for a single request. |
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.
Again, I don't see why this must be a guid. A lot of people are using ULIDs instead of UUIDs now and we should allow a service to use whatever value is useful for the service. Other than this, I agree with the rest of this guideline.
@@ -1292,6 +1292,52 @@ In addition to distributed tracing, Azure also uses a set of common correlation | |||
|x-ms-client-request-id |Both |Optional. Caller-specified value identifying the request, in the form of a GUID with no decoration such as curly braces (e.g. `x-ms-client-request-id: 9C4D50EE-2D56-4CD3-8152-34347DC9F2B0`). If the caller provides this header the service **must** include this in their log entries to facilitate correlation of log entries for a single request. Because this header can be client-generated, it should not be assumed to be unique by the service implementation. | |||
|x-ms-request-id |Response |Required. Service generated correlation id identifying the request, in the form of a GUID with no decoration such as curly braces. In contrast to the the `x-ms-client-request-id`, the service **must** ensure that this value is globally unique. Services should log this value with their traces to facilitate correlation of log entries for a single request. | |||
|
|||
<a href="#RP-API-MSD" name="RP-API-MSD"></a> | |||
### RP API More Secure Defaults Guidance |
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.
True, this document is not the right place for arm/rp guidelines. However, I think adding some of these guidelines to this document for data plane services is perfectly okay
<a href="#RP-API-MSD" name="RP-API-MSD"></a> | ||
### RP API More Secure Defaults Guidance | ||
|
||
The RP API MSD Guidelines are designed to ensure that any new RP API being developed is deployed from the outset with secure parameter default values. This proactive approach helps prevent non-secure defaults and maintains a high security standard from the very beginning of the API development process. |
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 have had numerous conversations about services exposing default values for properties and have been meaning to write a guideline about it. The guideline would go something like this: when a structure/model is first introduced by a service (V1 of structure), very few (if any) of the properties should have default values. Customer must explicitly select values for the properties. This prevents breaking changes should the service want to change a default since there are no defaults. Any new properties that are added to the structure/model in the future (V2+) must have default values in order to avoid breaking changes.
|
||
<a href="#rp-parameter-defining" name="rp-parameter-defining">:white_check_mark:</a> **DO** define parameters as explicit as possible: | ||
- Tagging parameters as required or not | ||
- If a parameter is not tagged as required and it has security implications, define a default value |
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 would say that there just shouldn't be a default and that the property is required - period.
<a href="#rp-parameter-defining" name="rp-parameter-defining">:white_check_mark:</a> **DO** define parameters as explicit as possible: | ||
- Tagging parameters as required or not | ||
- If a parameter is not tagged as required and it has security implications, define a default value | ||
- Defining the default value as a `key: "value"` pair |
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 just want to be clear that changing a default value is a breaking change and requires approval from the breaking change review board. This can all be avoided by not having defaults and instead have customers explicitly select what they desire.
This PR adds documentation for the RP API More Secure Defaults API writing guidance. It provides clear recommendations on how to structure and implement secure defaults within RP APIs to enhance security and consistency.