Added HANA database plugin #2811

Caiyeon · 2017-06-05T21:12:05Z

This set of commits adds HANA db as a database plugin, allowing for dynamic database user creation much like other database plugins.

This plugin allows for an {{expiration}} value in role definition. If defined in a role, upon credential generation, the plugin requests for HANA's server time, plus the default_ttl, and uses this value to create a user with a defined valid until field. The valid until field is a HANA side user property that ensures the user will be deactivated when the time is up, regardless of whether vault is available to revoke the user.

Revocation by default will perform a RESTRICT drop. This is a soft drop that will only drop users if they have no dependent objects. A CASCADE (hard) drop can be used by overwriting the revocation statements.

Renewing a lease will cause the plugin to request for HANA's time again, adding the current default_ttl, and changing the valid until property of the HANA user, to ensure the server will not deactivate the user until the lease is up.

Vendored dependencies:

https://github.com/SAP/go-hdb
https://github.com/golang/text (transitive, a dependency of go-hdb)

Acceptance tests have been written and executed. Documentation html pages have also been provided.

Let me know if there's anything missing

jefferai · 2017-06-05T21:26:23Z

@briankassouf @calvn Note - no rush as this will definitely not be in 0.7.3, but don't have another milestone up yet.

Caiyeon · 2017-06-05T21:31:58Z

Note: providing a {{expiration}} value in role definition combined with default_ttl=0 will cause:
* SQL HdbError 438 - invalid user parameter value: VALID UNTIL not allowed to be in the past

This is because the time is calculated just before sending, and thus the server will receive a time that is in the past.

This is documented in the documentation pages, where the expiration value is described. Let me know if you guys have a more elegant solution.

briankassouf · 2017-06-05T21:55:00Z

@Caiyeon I'll try and take a look a little later this week! But wanted to bring up that #2812 just changed the arguments of plugins slightly, so you'll probably want to keep an eye on that PR. I think it's likely to get merged today or tomorrow.

briankassouf · 2017-06-07T17:46:52Z

plugins/database/hana/hana.go

+
+	credsProducer := &credsutil.SQLCredentialsProducer{
+		DisplayNameLen: 32,
+		UsernameLen:    128,


RolenameLen and Separator should be specified here as well!

Will change. Seems like that functionality was added only recently

briankassouf · 2017-06-07T17:53:52Z

plugins/database/hana/hana.go

+
+// CreateUser generates the username/password on the underlying HANA secret backend
+// as instructed by the CreationStatement provided.
+func (h *HANA) CreateUser(statements dbplugin.Statements, usernamePrefix string, expiration time.Time) (username string, password string, err error) {


The function definition for CreateUser is changing in the next release you'll need to upgrade this to instead of receiving usernamePrefix, to receive a dbplugin.UsernameConfig object. See https://github.com/hashicorp/vault/blob/master/plugins/database/mysql/mysql.go#L85 for an example.

briankassouf · 2017-06-07T17:54:57Z

plugins/database/hana/hana.go

+	}
+
+	// HANA does not allow hyphens in usernames, and highly prefers capital letters
+	username = strings.Replace(username, "-", "_", -1)


This can be done by setting the Separator variable in the Credential producer object.

This in fact, cannot be done simply by setting the Separator variable.
If the displayname or rolename contains -, the username will be invalid for HANA db.
Therefore, even though Separator = "_", there is still an explicit conversion here.

briankassouf · 2017-06-07T17:55:52Z

plugins/database/hana/hana.go

+	// Most HANA configurations have password constraints.
+	// Prefix with A1a (user must change password upon login anyway)
+	password = strings.Replace(password, "-", "_", -1)
+	password = "A1a" + password


what does the "A1a" string do?

Most HANA dbs have a strict password policy. That is, they require one upper case, one numeric, and one lower case character (most widely used policy).

This prefix ensures that the newly generated password will conform to the policy.

briankassouf · 2017-06-07T17:58:44Z

plugins/database/hana/hana.go

+	// This ensures hana will deactivate the user when time is up, regardless of vault's actions
+	var expirationStr string
+	timeQuery := fmt.Sprintf("SELECT TO_NVARCHAR(add_seconds(CURRENT_TIMESTAMP,"+
+		"%f), 'YYYY-MM-DD HH24:MI:SS') FROM DUMMY", (expiration.Sub(time.Now())).Seconds())


Instead of adding the number of seconds you could just pass a formatted timestamp into the query, https://github.com/hashicorp/vault/blob/master/plugins/helper/database/credsutil/sql.go#L53 will format it correctly.

In fact, i'm not sure why an extra DB call is needed to get the expiration string?

Will change. This was a leftover from when the formatted timestamp function was not available

briankassouf · 2017-06-07T18:01:53Z

plugins/database/hana/hana.go

+	var expirationStr string
+	timeQuery := fmt.Sprintf("SELECT TO_NVARCHAR(add_seconds(CURRENT_TIMESTAMP,"+
+		"%f), 'YYYY-MM-DD HH24:MI:SS') FROM DUMMY", (expiration.Sub(time.Now())).Seconds())
+	if err := db.QueryRow(timeQuery).Scan(&expirationStr); err != nil {


Same comments as above.

briankassouf · 2017-06-07T18:05:41Z

plugins/database/hana/hana.go

+	}
+
+	// Renew user's valid until property field
+	stmt, err := tx.Prepare("ALTER USER " + username + " VALID UNTIL " + "'" + expirationStr + "'")


I don't know a lot about HanaDB, would there be a usecase for a custom renew statement?

If you are asking the use case for this particular SQL:

If expiration field is in the role's SQL, the generated credential will be set to expire when the lease expires on HANA server side. This means, in order to renew a credential, the expiration date that HANA keeps for that user must also be pushed back. Hence this ALTER USER sql.

If you are asking for the use case of potential custom renew statements that users may want to input:

I am unaware of any workflows where this would be needed. But if the convention is to make custom renew statements available, I can do that too.

Merge upstream

Caiyeon

Requested changes have been made

Caiyeon · 2017-06-08T18:29:31Z

plugins/database/hana/hana.go

+	}
+
+	// HANA does not allow hyphens in usernames, and highly prefers capital letters
+	username = strings.Replace(username, "-", "_", -1)


This in fact, cannot be done simply by setting the Separator variable.
If the displayname or rolename contains -, the username will be invalid for HANA db.
Therefore, even though Separator = "_", there is still an explicit conversion here.

Caiyeon · 2017-06-08T18:30:09Z

plugins/database/hana/hana.go

+	// Most HANA configurations have password constraints
+	// Prefix with A1a to satisfy these constraints. User will be forced to change upon login
+	password = strings.Replace(password, "-", "_", -1)
+	password = "A1a" + password


Password is prefixed with A1a to satisfy the most common HANA db password requirement (one upper case, one lower case, one numeric)

Caiyeon · 2017-06-08T18:30:50Z

plugins/database/hana/hana.go

+
+	// If expiration is in the role SQL, HANA will deactivate the user when time is up,
+	// regardless of whether vault is alive to revoke lease
+	expirationStr, err := h.GenerateExpiration(expiration)


Expiration string generation has been changed to the default way

Caiyeon · 2017-06-08T18:32:06Z

plugins/database/hana/hana_test.go

+
+	usernameConfig := dbplugin.UsernameConfig{
+		DisplayName: "test-test",
+		RoleName:    "test-test",


Displayname and rolename have been specifically injected with - in test, to make sure that these values are acceptable.

Caiyeon · 2017-06-28T17:27:01Z

@briankassouf are there any other requested changes? Hoping to see this in 0.7.4 if possible :)

calvn

Looking good! Left a couple of comments on the tests.

calvn · 2017-06-30T19:48:00Z

plugins/database/hana/hana_test.go

+
+func TestHANA_Initialize(t *testing.T) {
+	if os.Getenv("HANA_URL") == "" || os.Getenv("VAULT_ACC") != "1" {
+		return


I think t.SkipNow() would be more appropriate rather than assume the test passes. Same with the other test cases.

calvn · 2017-06-30T19:51:27Z

plugins/database/hana/hana_test.go

+)
+
+var (
+	testHANAImagePull sync.Once


This is unused and probably carried over from dockertest.v2 test code.

calvn · 2017-06-30T19:52:39Z

plugins/database/hana/hana_test.go

+	testHANAImagePull sync.Once
+)
+
+func TestHANA_Initialize(t *testing.T) {


Is it possible to test this using a HANA docker image, if one is available? Otherwise the test environment would somehow need to have a HANA DB instance running, with the proper environment variables set as well.

I'm not aware of any officially supported docker images, although there may be a couple of examples in the wild. One thing to note is that HANA is entirely in-memory, and (I believe) requires 64GB or above ram to function.

Just....to function? At all? 8-O

There is at least one unofficial image, with the express edition

I wonder though if, given the lack of easy testing with anything official, we should actually pull this into an external repo like the oracle plugin. That way we can have a separate release cycle with more rapid releases if there are issues.

What do you two think?

@Caiyeon let us discuss this bit internally, about where it should be hosted.

At the risk of making this unit test different from other db unit tests, could this be used?
https://github.com/DATA-DOG/go-sqlmock

I'm actually less concerned about the overall logic and more about actually testing it against a real server. It's a possibility, sure.

BTW, can you send me an email? It's my email address at my company dot com.

Sorry, I meant, my name.

* If env vars are not configured, tests will skip rather than succeed * Fixed some improperly named string variables

Caiyeon · 2017-07-07T18:31:46Z

@calvn @jefferai Any updates on how the testing should be done?

briankassouf · 2017-07-07T19:54:06Z

plugins/database/hana/hana.go

+	"github.com/hashicorp/vault/plugins"
+	"github.com/hashicorp/vault/plugins/helper/database/connutil"
+	"github.com/hashicorp/vault/plugins/helper/database/credsutil"
+	"github.com/hashicorp/vault/plugins/helper/database/dbutil"


I think that here you will need to import _ "github.com/SAP/go-hdb/driver"

briankassouf

Aside from my last comment this is looking good to go! Great work!

Caiyeon added 9 commits June 2, 2017 15:41

Added HANA dynamic secret backend

522bfdc

Added acceptance tests for HANA secret backend

fcc4541

Add HANA backend as a logical backend to server

b3775c5

Added documentation to HANA secret backend

6bc21d9

Added vendored libraries

7113ef3

Go fmt

608b3ff

Migrate hana credential creation to plugin

6dc2aaf

Removed deprecated hana logical backend

74c865b

Migrated documentation for HANA database plugin

0c0be33

jefferai added this to the 0.7.3 milestone Jun 5, 2017

jefferai requested review from briankassouf and calvn June 5, 2017 21:25

briankassouf reviewed Jun 7, 2017

View reviewed changes

jefferai modified the milestones: 0.7.3, 0.7.4 Jun 8, 2017

Caiyeon and others added 2 commits June 8, 2017 10:28

Merge pull request #1 from hashicorp/master

1d0e863

Merge upstream

Updated HANA DB plugin to use role name in credential generation

89e84cf

Caiyeon commented Jun 8, 2017

View reviewed changes

calvn reviewed Jun 30, 2017

View reviewed changes

Caiyeon added 2 commits July 6, 2017 15:26

Update HANA plugin tests

cb72c99

* If env vars are not configured, tests will skip rather than succeed * Fixed some improperly named string variables

Removed unused import

1f17846

Merge branch 'master' into master

4a66072

briankassouf reviewed Jul 7, 2017

View reviewed changes

briankassouf approved these changes Jul 7, 2017

View reviewed changes

jefferai approved these changes Jul 7, 2017

View reviewed changes

Import SAP hdb driver

341bbb0

briankassouf merged commit f92f4d4 into hashicorp:master Jul 7, 2017

jefferai modified the milestones: 0.7.4, 0.8.0 Jul 24, 2017

Added HANA database plugin #2811

Added HANA database plugin #2811

Conversation

Caiyeon commented Jun 5, 2017

jefferai commented Jun 5, 2017

Caiyeon commented Jun 5, 2017

briankassouf commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Caiyeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Caiyeon commented Jun 28, 2017

calvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvn Jun 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Caiyeon commented Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

briankassouf left a comment

Choose a reason for hiding this comment

calvn Jun 30, 2017 •

edited

Loading

Caiyeon commented Jul 7, 2017 •

edited

Loading