Audit sensitive attributes

[Hiieu]

Hi there 👋 ,

in this PR we've updated sensitive attributes.

[Integralist]

Interesting. I wouldn't necessarily have thought the email to be sensitive, but in some ways I guess this makes sense. @smaeda-ks any thoughts on that?

[bengesoff]

@Integralist @Hiieu and I were just discussing this and thought we'd raise this for discussion for two reasons: firstly the API docs call this field user whereas in the provider we call it email (potential breaking change to align these), and also the current provider description field calls it a "secret" whereas, as you said, we wouldn't have necessarily thought it to be sensitive.

Ps I planning to add more context before we marked it "ready for review" but you beat me to it 😉

[Integralist]

Thanks @bengesoff 🙂

So I think the naming should be aligned with the API, so email should change to user (@smaeda-ks would you agree?)

I would probably follow the API documentation in this case (which has a completely different description 🤦🏻 I always prefer to copy the description rather than write up our own custom description)...

Your Google Cloud Platform service account email address. The client_email field in your service account authentication JSON. Required.

Nothing about that suggests the user email is 'sensitive'? It's also returned in the API when doing a GET or UPDATE for example (which could well be a mistake on our part if this is supposed to be sensitive).

Reading about GCS 'service accounts' didn't suggest to me that the user email is sensitive either: https://cloud.google.com/iam/docs/service-accounts

[smaeda-ks]

@Integralist

So I think the naming should be aligned with the API, so email should change to user (@smaeda-ks would you agree?)

Yes

As for the sensitive of the email address attribute, I tend to agree that we don't necessary to make this change.

[Hiieu]

Thanks @Integralist and @smaeda-ks , I'll change the email attribute to user and desensitise it.
While I have your attention, I've removed the sensistive attribute from thetls_ca_cert and tls_client_cert fields in fastly/block_fastly_service_v1_httpslogging.go to keep consistency across the blocks.
But...now I've noticed that:

1. block_fastly_service_v1_logging_elasticsearch.go
2. block_fastly_service_v1_logging_kafka.go

have the same fields set as sensitive, so I wonder if we should desensitise these blocks as well? 🤔

[smaeda-ks]

@Hiieu

have the same fields set as sensitive, so I wonder if we should desensitise these blocks as well? 🤔

Yes, client CA and cert are generally not considered as sensitive, but client "key". So I'm +1 with desensitise those tls attributes as well. Thanks for flagging that!

[Hiieu]

All comments have been addressed:

1. Changed email to user
2. Desensitised the ca and client certs

The PR is ready for review.

[Integralist]

Due to the change of email to user this sounds like a breaking change to me as it'll affect a user's configuration.

[Integralist]

LGTM. But because the email change to user is a breaking change, maybe we move that change to a separate PR otherwise these 'sensitive' audit changes won't be mergeable until we're ready to release a 1.0.0 release.

Thoughts?

[Hiieu]

@Integralist, sounds good, I can move the email change to a separate PR.

[Hiieu]

The PR for the email change: #461

[Integralist]

Thanks @Hiieu let me know if this PR is ready to merge.

[Hiieu]

@Integralist, it's ready to be merged 👍