Skip to content
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 security label resource #365

Closed
wants to merge 3 commits into from
Closed

Conversation

jbunting
Copy link
Contributor

I've added a security label resource. This allows to create and manage security labels in postgres (https://www.postgresql.org/docs/current/sql-security-label.html).

Unfortunately, in order to test the security label operations, a "security label provider" has to be installed. My first commit in this PR does that, but to be honest its a bit onerous and it copies a little bit of code from postgres's test tree. I'm fully open to thoughts on how to better handle that part.

I believe this also addresses #276 .

@djr747
Copy link

djr747 commented Jan 15, 2024

@cyrilgdn is possible without an actual valid test Azure Database for PostgreSQL to get this PR merged? Also interested in Entra Auth principals being supported which requires Security Label support.

@Teabeats
Copy link

@jbunting Can you bring in jbunting#1 into this PR to add a doc page for this resource?

Copy link

@Teabeats Teabeats left a comment

Choose a reason for hiding this comment

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

Copy link

@jarpoole jarpoole left a comment

Choose a reason for hiding this comment

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

Thank you for contributing @jbunting!

Update: PGResourceFunc(resourcePostgreSQLSecurityLabelUpdate),
Delete: PGResourceFunc(resourcePostgreSQLSecurityLabelDelete),
Importer: &schema.ResourceImporter{
StateContext: schema.ImportStatePassthroughContext,
Copy link

Choose a reason for hiding this comment

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

Not well versed in writing Terraform providers but does this mean the resource supports importing? I personally feel importing is quite important to help with adoption and so it this is the case I think it makes sense to add an import example similar to a lot of the other resources?

image

Or even simpler:
image

@glangho
Copy link

glangho commented Jul 3, 2024

Would be great to have this officially.

Verified working e2e with this temporary release: https://registry.terraform.io/providers/Teabeats/postgresql/latest/docs/resources/postgresql_security_label

Has this been tested with an azure database? I tested with a basic role like "my_test_role" and it works fine. It fails on any azure entra managed id.

# postgresql_security_label.entra_aadauth["my-entra-id"] will be created
+ resource "postgresql_security_label" "entra_aadauth" {
        + id                 = (known after apply)
        + label            = "aadauth,oid=*****,type=service"
        + label_provider     = "pgaadauth"
        + object_name       = "my-entra-id"
        + object_type         = "role"
    }

Plan: 1 to add, 0 to change, 0 to destroy.
postgresql_security_label.entra_aadauth["my-entra-id"]: Creating...

Error: Provider produced inconsistent result after apply

When applying changes to
postgresql_security_label.entra_aadauth["ar-data-services01-devsecops-dev"],
provider "provider[\"registry.terraform.io/teabeats/postgresql\"]" produced
an unexpected new value: Root resource was present, but now absent.

When I query pg_seclabels I notice that all the entra ids are literally quoted in the objname field vs. the basic role I created a security label for is not. Ex., "my-entra-id" vs my_test_role

My thought is, the resource gets created successfully but after it attempts to read it back it's looking for my-entra-id but the value is actually "my-entra-id". I can confirm this by a simple select * from pg_seclabels where objname = 'my-entra-id' which returns nothing while select * from pg_seclabels where objname = '"my-entra-id"' returns a value.

Edit: maybe it's not so much entra id as it is ids with hyphens. I just noticed that even the individual users and aad group assigned as server admins are quoted.

@glangho
Copy link

glangho commented Jul 11, 2024

Periods ('.') and at ('@') symbols (i.e., users in entra) also have the same problems as dashes so [email protected] gets saved in the security labels table as "[email protected]", again failing to match after the apply takes place.

@DzmtrShlh
Copy link

Hello everyone!

Can confirm that I ran into the same issue as @glangho when I was testing postgresql_security_label resource. Should mention that resource was created (group in my case) and I was able to login as a member of this group.

Also interested in Microsoft Entra authentication for Azure Database for PostgreSQL - Flexible Server that requires postgresql_security_label resource to be introduced in official release.

@cyrilgdn cyrilgdn force-pushed the main branch 3 times, most recently from f2c2e47 to dea1401 Compare September 8, 2024 17:06
}
log.Printf("[WARN] PostgreSQL security label Create")
label := d.Get(securityLabelLabelAttr).(string)
if err := resourcePostgreSQLSecurityLabelUpdateImpl(db, d, pq.QuoteLiteral(label)); err != nil {

Choose a reason for hiding this comment

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

few lines contain quoteliteral(label), what might cause the issue with quotes in the label Objname reported before. Could you please try to make it work - to create objname/label without quotes in postgresql?

Choose a reason for hiding this comment

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

query := "SELECT objtype, objname, provider, label FROM pg_seclabels WHERE objtype = $1 and objname = quote_literal($2) and provider = $3"

I'm no go language developer to test it, but based on the chatgpt this should fix the issue

Copy link
Contributor

@stanleyz stanleyz left a comment

Choose a reason for hiding this comment

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

@jbunting possible to update this and hopefully we can get this merged?

I am happy to help if you have other plans.

}
defer deferredRollback(txn)

query := "SELECT objtype, objname, provider, label FROM pg_seclabels WHERE objtype = $1 and objname = $2 and provider = $3"
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned by other numbers, you need quote_ident on objname.

SELECT objtype, objname, provider, label FROM pg_seclabels WHERE objtype = $1 and objname = quote_ident($2) and provider = $3

@stanleyz
Copy link
Contributor

Thanks @jbunting for the great work, I've since raised #482 based on this PR.

@cyrilgdn
Copy link
Owner

Closing in favor of #482 that supersedes it.

@cyrilgdn cyrilgdn closed this Oct 23, 2024
cyrilgdn added a commit that referenced this pull request Oct 30, 2024
This PR is based on the great job @jbunting did in #365, which fixed the
quoted identifier for object name and provider in the `pg_seclabels`
table.

Additional changes:

1. Update the doc to explain the import process

---------

Co-authored-by: Jared Bunting <[email protected]>
Co-authored-by: Cyril Gaudin <[email protected]>
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.

9 participants