Skip to content

Conversation

@strantalis
Copy link
Member

@strantalis strantalis commented Aug 16, 2024

Should Resolve: #751

In this pull request, I add the masq package to provide developers the ability to mask struct fields by applying the tag masq:"secret".

I also removed logging of the services field from the config object. This change forces anyone adding additional services to log their own service-specific configuration separately. The main reason for this is that there are too many unknowns regarding the understanding of the config structure. However, if developers leverage this logger and apply the struct tags, they can redact sensitive fields.

For more information, visit https://github.com/m-mizutani/masq.

Example Loading Config Log

time=2024-08-16T12:12:18.497Z level=DEBUG msg="config loaded" config.dev_mode=true config.db.host=localhost config.db.port=5432 config.db.database=opentdf config.db.user=postgres config.db.password=[REDACTED] config.db.sslmode=prefer config.db.schema=opentdf config.db.runMigrations=true config.db.verifyConnection=true config.logger="{Level:trace Output:stdout Type:text}" config.mode=[all] config.sdk_config="{Endpoint: Plaintext:false ClientID: ClientSecret:[REDACTED]}" config.server.auth="{Enabled:false PublicRoutes:[] AuthNConfig:{EnforceDPoP:false Issuer:https://virtru-keylcoak.ngrok.pizza/auth/realms/opentdf Audience:http://localhost:8080 Policy:{Default: RoleClaim: RoleMap:map[] Csv: Model:} CacheRefresh:15m DPoPSkew:1h0m0s TokenSkew:1m0s}}" config.server.grpc="{ReflectionEnabled:true MaxCallRecvMsgSizeBytes:4194304 MaxCallSendMsgSizeBytes:4194304}" config.server.cryptoProvider="{Type:standard StandardConfig:{Keys:[{Algorithm:rsa:2048 KID:r1 Private:kas-private.pem Certificate:kas-cert.pem Usage: Purpose:} {Algorithm:ec:secp256r1 KID:e1 Private:kas-ec-private.pem Certificate:kas-ec-cert.pem Usage: Purpose:}] RSAKeys:map[] ECKeys:map[]}}" config.server.tls="{Enabled:false Cert: Key:}" config.server.cors="{Enabled:false AllowedOrigins:[*] AllowedMethods:[GET POST PATCH PUT DELETE OPTIONS] AllowedHeaders:[ACCEPT Authorization Content-Type X-CSRF-Token X-Request-ID] ExposedHeaders:[Link] AllowCredentials:true MaxAge:3600}" config.server.port=8080 config.server.host="" config.server.enablePprof=false

Example Service Log

time=2024-08-16T12:12:18.676Z level=DEBUG msg="entity_resolution configuration" namespace=entityresolution config="{URL:http://localhost:8888/auth Realm:opentdf ClientID:tdf-entity-resolution ClientSecret:[REDACTED] LegacyKeycloak:true SubGroups:false InferID:{From:{ClientID:false Email:true Username:true}}}"

@strantalis strantalis changed the title Masq logger fix(core): cleanup sensitive info being logged from configuration Aug 16, 2024
@strantalis strantalis marked this pull request as ready for review August 16, 2024 12:28
@strantalis strantalis requested review from a team as code owners August 16, 2024 12:28
jrschumacher
jrschumacher previously approved these changes Aug 16, 2024
Copy link
Member

@jrschumacher jrschumacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good start but I think we need a little more to prevent accidental leaks. Areas of concern:

  • copying config to an interface and printing
  • marshaling and printing
  • extra config props that are not mapped to a struct

We can address those at another time.

@strantalis
Copy link
Member Author

Because we detected performance issues adding masq. I have decided to remove it for the time being. Instead we just use the standard LogValue method to remove sensitive content when logging configuration.

@strantalis strantalis enabled auto-merge August 17, 2024 14:10
@jrschumacher
Copy link
Member

Specifically, masq used reflection. When logging large objects like request bodies this caused severe performance issues.

@strantalis strantalis added this pull request to the merge queue Aug 20, 2024
Merged via the queue into opentdf:main with commit 2b6cf62 Aug 20, 2024
@strantalis strantalis deleted the masq-logger branch August 20, 2024 14:16
github-merge-queue bot pushed a commit that referenced this pull request Aug 20, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.4.19](service/v0.4.18...service/v0.4.19)
(2024-08-20)


### Features

* **core:** add RPCs to namespaces service to handle assignment/removal
of KAS grants ([#1344](#1344))
([ee47d6c](ee47d6c))
* **core:** Adds key ids to kas registry
([#1347](#1347))
([e6c76ee](e6c76ee))
* **core:** further support in policy for namespace grants
([#1334](#1334))
([d56231e](d56231e))
* **core:** support grants to namespaces, definitions, and values in
GetAttributeByValueFqns
([#1353](#1353))
([42a3d74](42a3d74))
* **core:** validate kas uri
([#1351](#1351))
([2b70931](2b70931))
* **policy:** 1277 protos and service methods for Resource Mapping
Groups operations
([#1343](#1343))
([570f402](570f402))
* **sdk:** Load KAS keys from policy service
([#1346](#1346))
([fe628a0](fe628a0))
* **sdk:** public client and other enhancements to well-known SDK
functionality ([#1365](#1365))
([3be50a4](3be50a4))


### Bug Fixes

* **authz:** Add http routes for authorization to casbin policy
([#1355](#1355))
([3fbaf59](3fbaf59))
* **core:** align keycloak provisioning in one command
([#1381](#1381))
([c3611d2](c3611d2)),
closes [#1380](#1380)
* **core:** align policy kas grant assignments http gateway methods with
actions ([#1299](#1299))
([031c6ca](031c6ca))
* **core:** Autobump service
([#1340](#1340))
([3414670](3414670))
* **core:** Autobump service
([#1369](#1369))
([2ac2378](2ac2378))
* **core:** Autobump service
([#1403](#1403))
([8084e3e](8084e3e))
* **core:** Autobump service
([#1405](#1405))
([74a7f0c](74a7f0c))
* **core:** bump go version to 1.22
([#1407](#1407))
([c696cd1](c696cd1))
* **core:** cleanup sensitive info being logged from configuration
([#1366](#1366))
([2b6cf62](2b6cf62))
* **core:** policy kas grants list (filter params and namespace grants)
([#1342](#1342))
([f18ba68](f18ba68))
* **core:** policy migrations timestamps merge order
([#1325](#1325))
([2bf4290](2bf4290))
* **sdk:** align sdk with platform modes
([#1328](#1328))
([88ca6f7](88ca6f7))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

config loaded debug statement logs secrets

4 participants