Password shown in plain text in Broker Describe output #743
Assignees
Labels
kind/enhancement
Categorizes issue or PR as related to existing feature enhancements.
Milestone
Comments
phanimullapudi
added
the
kind/enhancement
RedbackThomson
added
kind/bug
|
Working on #748 as a pre-requisite step for this. |
jaypipes
added a commit
to jaypipes/ack-code-generator
that referenced
this issue
Apr 28, 2021
When designing the `pkg/generate/config.FieldConfig` object, we really only thought about (and tested for) "top-level fields". In other words, fields that were on the CR's `Spec` or `Status` objects. However, as we've added more configurability to the code-generator regarding fields -- including marking fields as printable or secrets -- we're now limited substantially by our initial implementation that only allows referring to top-level fields. For example, we cannot mark a field as being a secret field if that field is a subfield of one of these top-level fields. Concretely, this means that with the existing generator configuration, we cannot support configuring AmazonMQ's `Broker` CRD's `Spec.Users.Password` field as a SecretKeyReference because `Password` is not a top-level field. It is instead a field *of* a top-level field -- a "nested field" -- and therefore our simplistic configuration system isn't able to tell the code generator to replace the `string` Go type with `*ackv1alpha1.SecretKeyReference` like it [does][secref-inject] for top-level fields. [secref-inject]: https://github.com/aws-controllers-k8s/code-generator/blob/94186d92e778792ccba11b5db3478e037256b36b/pkg/model/field.go#L61-L64 The solution to this dilemma is contained in this PR and involves reworking our generator model in a couple ways: 1) The `pkg/model.CRD` struct now has a `Fields` attribute to go along with the existing `SpecFields` and `StatusFields` attributes. `CRD.Fields` is a `map[string]*Field` like `SpecFields` and `StatusFields`, but unlike those attributes, the `Fields` map keys are field *paths* and not the original aws-sdk-go Shape member names. When inspecting the AWS service API model as we do in `pkg/generate.Generator:GetCRDs()`, we now construct a field path for each `pkg/model.Field` object that corresponds to "where" in the CR the field is. For example, `Spec.Name` or `Status.BrokerInstances` 2) The `pkg/generate.Generator` has been updated with a `processNestedFields()` method to populate the new field-path-keyed `CRD.Fields` map with nested fields, making it possible to refer in our generator config, by field path, to fields that are not directly in the Status or Spec object. A following patch will update the `Generator.GetTypeDefs()` method to look up field configuration by this new field-path-based map and replace `string` with `*ackv1alpha1.SecretKeyReference` when the field is a secret field (Issue aws-controllers-k8s/community#743). Issue aws-controllers-k8s/community#748
jaypipes
added a commit
to jaypipes/ack-code-generator
that referenced
this issue
Apr 28, 2021
When designing the `pkg/generate/config.FieldConfig` object, we really only thought about (and tested for) "top-level fields". In other words, fields that were on the CR's `Spec` or `Status` objects. However, as we've added more configurability to the code-generator regarding fields -- including marking fields as printable or secrets -- we're now limited substantially by our initial implementation that only allows referring to top-level fields. For example, we cannot mark a field as being a secret field if that field is a subfield of one of these top-level fields. Concretely, this means that with the existing generator configuration, we cannot support configuring AmazonMQ's `Broker` CRD's `Spec.Users.Password` field as a SecretKeyReference because `Password` is not a top-level field. It is instead a field *of* a top-level field -- a "nested field" -- and therefore our simplistic configuration system isn't able to tell the code generator to replace the `string` Go type with `*ackv1alpha1.SecretKeyReference` like it [does][secref-inject] for top-level fields. [secref-inject]: https://github.com/aws-controllers-k8s/code-generator/blob/94186d92e778792ccba11b5db3478e037256b36b/pkg/model/field.go#L61-L64 The solution to this dilemma is contained in this PR and involves reworking our generator model in a couple ways: 1) The `pkg/model.CRD` struct now has a `Fields` attribute to go along with the existing `SpecFields` and `StatusFields` attributes. `CRD.Fields` is a `map[string]*Field` like `SpecFields` and `StatusFields`, but unlike those attributes, the `Fields` map keys are field *paths* and not the original aws-sdk-go Shape member names. When inspecting the AWS service API model as we do in `pkg/generate.Generator:GetCRDs()`, we now construct a field path for each `pkg/model.Field` object that corresponds to "where" in the CR the field is. For example, `Spec.Name` or `Status.BrokerInstances` 2) The `pkg/generate.Generator` has been updated with a `processNestedFields()` method to populate the new field-path-keyed `CRD.Fields` map with nested fields, making it possible to refer in our generator config, by field path, to fields that are not directly in the Status or Spec object. A following patch will update the `Generator.GetTypeDefs()` method to look up field configuration by this new field-path-based map and replace `string` with `*ackv1alpha1.SecretKeyReference` when the field is a secret field (Issue aws-controllers-k8s/community#743). Issue aws-controllers-k8s/community#748
jaypipes
added a commit
to jaypipes/ack-code-generator
that referenced
this issue
Apr 28, 2021
When designing the `pkg/generate/config.FieldConfig` object, we really only thought about (and tested for) "top-level fields". In other words, fields that were on the CR's `Spec` or `Status` objects. However, as we've added more configurability to the code-generator regarding fields -- including marking fields as printable or secrets -- we're now limited substantially by our initial implementation that only allows referring to top-level fields. For example, we cannot mark a field as being a secret field if that field is a subfield of one of these top-level fields. Concretely, this means that with the existing generator configuration, we cannot support configuring AmazonMQ's `Broker` CRD's `Spec.Users.Password` field as a SecretKeyReference because `Password` is not a top-level field. It is instead a field *of* a top-level field -- a "nested field" -- and therefore our simplistic configuration system isn't able to tell the code generator to replace the `string` Go type with `*ackv1alpha1.SecretKeyReference` like it [does][secref-inject] for top-level fields. [secref-inject]: https://github.com/aws-controllers-k8s/code-generator/blob/94186d92e778792ccba11b5db3478e037256b36b/pkg/model/field.go#L61-L64 The solution to this dilemma is contained in this PR and involves reworking our generator model in a couple ways: 1) The `pkg/model.CRD` struct now has a `Fields` attribute to go along with the existing `SpecFields` and `StatusFields` attributes. `CRD.Fields` is a `map[string]*Field` like `SpecFields` and `StatusFields`, but unlike those attributes, the `Fields` map keys are field *paths* and not the original aws-sdk-go Shape member names. When inspecting the AWS service API model as we do in `pkg/generate.Generator:GetCRDs()`, we now construct a field path for each `pkg/model.Field` object that corresponds to "where" in the CR the field is. For example, `Spec.Name` or `Status.BrokerInstances` 2) The `pkg/generate.Generator` has been updated with a `processNestedFields()` method to populate the new field-path-keyed `CRD.Fields` map with nested fields, making it possible to refer in our generator config, by field path, to fields that are not directly in the Status or Spec object. A following patch will update the `Generator.GetTypeDefs()` method to look up field configuration by this new field-path-based map and replace `string` with `*ackv1alpha1.SecretKeyReference` when the field is a secret field (Issue aws-controllers-k8s/community#743). Issue aws-controllers-k8s/community#748
jaypipes
added a commit
to jaypipes/ack-code-generator
that referenced
this issue
Apr 29, 2021
In this patch, we post-process the `pkg/model.TypeDef` objects that are constructed by the code generator for nested fields of type "structure". This post-processing is used to replace the GoType of an Attr representing a Secret field in a nested parent field. For example, the AmazonMQ `Broker` resource has a `Spec.Users` top-level field. This top-level field is of type `[]*User`. The `User` Go struct type is constructed from a `pkg/model.TypeDef` object that describes the struct's attributes. One of those struct attributes is called `Password`, and we want to replace the Go type for this `Attr` from `string` to `*ackv1alpha1.SecretKeyReference`. In order to do this Go type replacement, we need to search through the TypeDef objects, looking for the parent field TypeDef, then look through that TypeDef's Attr collection for the Attr with the same name as our Field that has a FieldConfig.IsSecret setting of `true`. Issue aws-controllers-k8s/community#743
jaypipes
added a commit
to jaypipes/ack-code-generator
that referenced
this issue
Apr 29, 2021
In this patch, we post-process the `pkg/model.TypeDef` objects that are constructed by the code generator for nested fields of type "structure". This post-processing is used to replace the GoType of an Attr representing a Secret field in a nested parent field. For example, the AmazonMQ `Broker` resource has a `Spec.Users` top-level field. This top-level field is of type `[]*User`. The `User` Go struct type is constructed from a `pkg/model.TypeDef` object that describes the struct's attributes. One of those struct attributes is called `Password`, and we want to replace the Go type for this `Attr` from `string` to `*ackv1alpha1.SecretKeyReference`. In order to do this Go type replacement, we need to search through the TypeDef objects, looking for the parent field TypeDef, then look through that TypeDef's Attr collection for the Attr with the same name as our Field that has a FieldConfig.IsSecret setting of `true`. Issue aws-controllers-k8s/community#743
jaypipes
added a commit
to jaypipes/ack-code-generator
that referenced
this issue
May 1, 2021
Implements the `pkg/generate/code.SetSDK()` function for all nested fields that are SecretKeyReference types. This involved passing a field path down into the recursive `setSDKForXXX()` functions to keep track of where we were in the processing of nested fields. We look up whether a field is a SecretKeyReference by calling the `pkg/model.CRD:IsSecretField()` method, which accepts a field path, which is why we needed to pass this field path down into the recursive code generators. Issue aws-controllers-k8s/community#743
jaypipes
added a commit
to jaypipes/ack-code-generator
that referenced
this issue
May 3, 2021
Implements the `pkg/generate/code.SetSDK()` function for all nested fields that are SecretKeyReference types. This involved passing a field path down into the recursive `setSDKForXXX()` functions to keep track of where we were in the processing of nested fields. We look up whether a field is a SecretKeyReference by calling the `pkg/model.CRD:IsSecretField()` method, which accepts a field path, which is why we needed to pass this field path down into the recursive code generators. Issue aws-controllers-k8s/community#743
|
This is in v0.0.3 release of mq-controller (being released today) |
jaypipes
added a commit
to jaypipes/ack-mq-controller
that referenced
this issue
May 19, 2021
Replaces the string `Password` field on the nested `Broker.Spec.Users` field with a SecretKeyReference so that plain text passwords are not passed in Broker CRDs. This commit introduces a manual change to the config/rbac/cluster-role-controller.yaml file to allow the controller to read Secrets from the k8s API server. See aws-controllers-k8s/community#745 for why that was necessary. Issue aws-controllers-k8s/community#743
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Is your feature request related to a problem?
When we describe kubectl describe broker/brokername - the password is shown as plain text.
Describe the solution you'd like
It should be hidden like any other secret.
Describe alternatives you've considered
N.A
Users: Console Access: true Groups: Password: adminpassneeds12chars Username: admin@jaypipes @mcmains7812
The text was updated successfully, but these errors were encountered: