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

[FEATURE REQUEST] Provide a way to set SQLServer-specific connection-state via utility methods on SQLServerConnection, esp sp_setapprole since it faces 'special' issues. #1540

Open
dtbullock opened this issue Mar 15, 2021 · 5 comments
Labels
Backlog The topic in question has been recognized and added to development backlog Enhancement An enhancement to the driver. Lower priority than bugs.

Comments

@dtbullock
Copy link

dtbullock commented Mar 15, 2021

Is your feature request related to a problem? If so, please give a short summary of the problem and how the feature would resolve it

It's impossible to use sp_setapprole and sp_unsetapprole using a JDBC CallableStatement because when the client code does this:

 conn.prepareCall(
     "{? = call sys.sp_setapprole(@rolename = ?, @password = ?, @fCreateCookie = true, @cookie = ?)}"
 )

the JDBC driver always tries to sp_prepexec, but sp_setapprole can't be used in that way ... per the 'Remarks' of sp_setappRole (Transact-SQL):

sp_setapprole can be executed only by direct Transact-SQL statements, at the ad hoc level and not within another stored procedure, trigger or within a user-defined transaction.

... nor when the SQL is given to sp_prepexec. We get an exception:

'Stored procedure 'sys.sp_setapprole' can only be executed at the ad hoc level.'

This is an impedance mismatch between the JDBC view of the world, where a CallableStatement inherits from PreparedStatement, but sys.sp_setapprole is 'special' and cannot be prepared.

Describe the preferred solution

In this case, we don't care about re-using query plans, nor sending multiple updates in a batch. We just care getting/setting in/out parameters.

Since sys.sp_setapprole and sys.sp_setapprole are so fussy about how they are called, the JDBC driver should provide special treatment for these stored procedures, and simply pass them to EXEC, not try to sp_prepexec them.

Alternatively (and better), SQLServerConnection could offer 'pushAppRole(String)' and 'popAppRole()' methods. Working with this DBMS-specific feature via a DBMS-specific API is just fine.

Describe alternatives you've considered

It is possible to work-around and get the cookie using conn.createStatement(sql).executyQuery() where sql is:

  DECLARE @savedcookie as VARBINARY(8000)
  EXEC sys.sp_setapprole @rolename = 'myrole', @password = 'mypassword', @fCreateCookie = true, @cookie = @savedcookie out;
  SELECT @savedcookie;

and then later follow this up with:

  DECLARE @result AS BIT;
  EXEC @result = sys.sp_unsetapprole @cookie = 0x00...FF;
  SELECT @result;

... with notable API-ugliness forced upon the user to:

  • manually assemble a string
  • convert the cookie bits retrieved earlier, to an SQL server hex literal

Additional context

Sure, application roles are of limited value, providing mainly a means of keeping state which is specific to an application hidden from users when they access the database using a different application.

But the OAuth use-case, where an application collects access-tokens on behalf of many users who have chosen to trust that application (ie. the 'client' in OAuth terminology) is exactly the scenario which benefits from an application-role.

Reference Documentations/Specifications

sp_setappRole (Transact-SQL)
https://stackoverflow.com/questions/6833278/jdbc-set-approle/6944693#6944693

@dtbullock dtbullock added the Enhancement An enhancement to the driver. Lower priority than bugs. label Mar 15, 2021
@peterbae
Copy link
Contributor

Hi @dtbullock, thanks for submitting this issue. I see that you've offered two solutions to enhance the driver, where the first solution is to treat sp_setapprole specially, and the other is to add a specific API to handle this.

As for handling sp_setapprole specially, this would mean that the driver would have to do string parsing on the user's query during runtime (on every query) to see if the query is doing sp_setapprole, which would introduce a performance degradation on something that is rarely called. As for the other solution of adding a special API, all the API would be doing is to call what you've suggested in the workaround, while cluttering the codebase with something that the user can call themselves (and I don't believe anyone else has asked for this API to be implemented).

Therefore, I believe that it would be best if you were to keep using the workaround you have, instead of having the driver do String parsing or add an API that replicates your workaround, given that this sp_setapprole procedure is not called very often by most users. Let me know if you have any questions regarding this.

@dtbullock
Copy link
Author

dtbullock commented Mar 17, 2021

Hi @peterbae thanks for giving this some thought. I agree that if the driver doesn't already parse stuff, then it would be foolishness to make it do so to support this-particular use-case.

I do think it would be worth 'cluttering the API' though. There are quite a few responsibilities that the hapless app author must assume:

  • because we can't use IN/OUT parameters of CallableStatement, we have to manually code the VARBINARY cookie bytes to an SQL Server Hex Literal for sp_unsetapprole
  • we have to stash the cookie somewhere safe between sp_setapprole and sp_unsetapprole
  • we have to make sure we call sp_unsetapprole before the connection is returned to the pool
  • we are forced to write cruddy SQL statements by hand, because my fancy ORM/SQL-DSL simply deals with DML statements and NOT with these DBMS-specific connection-state-affecting SQL statements.

Like autoCommit and transactionIsolation, the appRole is a connection-wide property which needs special treatment in connection-pooling situations so that pool-controlling code can restore the connection to a re-use state.

In fact, any sort of SQL-Server-specific feature which affects the connection-state would profit from this: assumed identities, opened symmetric keys, etc. It'd make my life as a user of the database much simpler if such nothing-to-do-with-data, DBMS-vendor-specific, connection-state travelled with the Connection object itself, and was eminently controllable by a 'framework' that I didn't have to write. Do you want to add value to your technology stack, and increase customer stickiness to SQL Server? Or would you rather not 'clutter' the API?

We're talking about:

conn.pushAppRole("myrole", "mypass") // can be called multiple times, pushes cookie onto LIFO stack
conn.popAppRole() // pops from LIFO stack of cookies pushed by pushAppRole
conn.unsetAppRole() // calls sp_unsetapprole with 'cookie 0' and discards the LIFO stack

In contrast, here is what I have to do. Resarching, prototyping, and polishing all this took me away from solving my business problems for double-digit hours. It is still incomplete in regards to exception-handling, but I've got an app to write and it's not bothering me yet.

// lang: Kotlin

// I use the below something like:

        datasource.exec() // extension method to get initial JdbcExec
            .withLegacyRole(appRole, appRolePwd)   // sp_setapprole / sp_unsetapprole, stores cookie
            .withSymmKey(storeConfig.symKeyName, storeConfig.certName) // open symmetric key x by certificate y
            .withTransaction(false, Connection.TRANSACTION_READ_COMMITTED) // con.autoCommit, con.transactionIsolation
            .execute { conn -> 
                   /* use the connection now it has been prepared, and will be unprepared when I am done */ 
            }

/// JdbcExec and friends are my strategy for having a 'pre' step that must 
// pass state to a 'post 'step  after the 'DML of interest' has been executed
// all the 'pre' closures 'up the chain' get executed, possibly outputting some value for its corresponding 'post'
// then the 'code block that relies on the connection state' get executed
// the the 'post' closures 'up the chain' get executed, accepting any state from the corresponding 'pre' closure

interface JdbcExec<R> {   // R is a 'result'
    fun execute(action: (Connection) -> R): R
    fun withTransaction(wantedAuto: Boolean, wantedIso: Int): JdbcExec<R>
    fun withLegacyRole(role: String, password: String): JdbcExec<R>
    fun withSymmKey(keyname: String, certname: String): JdbcExec<R>
    fun withRole(role: String, password: String): JdbcExec<R>
}

interface JdbcExecPriv<R> : JdbcExec<R> {  // hack to eliminate <T> from JdbcExccImpl 
    fun doPre()
    fun doPost()
}

private class JdbcExecImpl<T, R> ( // T keeps pre and post together.  R is result.
    private val conn: Connection,
    private val pre: () -> T,
    private val post: (T) -> Unit,
    private val enclosing: JdbcExecPriv<R>?
) : JdbcExecPriv<R> {

[...] 

// Example chain-method concerning app role.  I also have withTransaction and withSymmetricKey
// ... and anticipate that others will be needed in the future if I am to make 
// use of SQL Server's features, and not just treat it like a 'dumb DML' repository

    override fun withLegacyRole(role: String, password: String): JdbcExecImpl<ByteArray, R> {
        return JdbcExecImpl(
            conn,
            pre = {
                conn.createStatement().use { stmt ->
                    stmt.executeQuery(
                        """
                    DECLARE @later as VARBINARY(8000)
                    EXEC sys.sp_setapprole @rolename = '$role', @password = '$password', @fCreateCookie = true, @cookie = @later out;
                    SELECT @later;
                    """.trimIndent()
                    ).use { resultSet ->
                        when (resultSet.next()) {
                            true -> resultSet.getBytes(1)
                            false -> byteArrayOf()
                        }
                    }
                }
            },
            post = { cookieBytes ->
                conn.createStatement().use { stmt ->
                    stmt.executeQuery(
                        """
                    DECLARE @result AS BIT;
                    EXEC @result = sys.sp_unsetapprole @cookie = ${cookieBytes.asSQLServerLiteral()};
                    SELECT @result;
                    """.trimIndent()
                    ).use { resultSet ->
                        when (resultSet.next()) {
                            true -> !resultSet.getBoolean(1)  // sp_unsetapprole returns 0 for success
                            else -> false
                        }
                    }
                }
            },
            enclosing = this
        )
    }

[...]

    private var baggage: T? = null

    override fun doPre() {
        enclosing?.doPre()
        baggage = pre()
    }

    override fun doPost() {
        post(baggage!!)
        enclosing?.doPost()
    }

    override fun execute(action: (Connection) -> R): R { // action!  invoked by the 'tail'
        doPre()
        val result = action(conn)
        doPost()
        return result
    }

}

not to forget the supporting binary-to-hex:

private const val hexDigits = "0123456789ABCDEF"

fun ByteArray.asSQLServerLiteral(): String {
    val hexChars = CharArray(this.size * 2)
    for (j in this.indices) {
        // promote to 32-bit with left-most 24 bits all 0, to give a positive
        val v = this[j].toInt() and 0xFF

        // 4 bits from the right-most 8 bits each locate the right hex char from the array of 16
        hexChars[j * 2] = hexDigits[v ushr 4]  // first hex char, uses bits 5-8
        hexChars[j * 2 + 1] = hexDigits[v and 0x0F] // second hex char, uses bits 1-4
    }
    return "0x${String(hexChars)}"
}

```

@dtbullock dtbullock changed the title [FEATURE REQUEST] Treat sp_setapprole and sp_unsetapprole specially, to avoid the futile attempt to sp_prepexec them when using Connection#prepareCall() [FEATURE REQUEST] Provide a way to set DMBS-specific connection-state via utility methods on SQLServerConnection, esp sp_setapprole since it faces 'special' issues. Mar 17, 2021
@dtbullock dtbullock changed the title [FEATURE REQUEST] Provide a way to set DMBS-specific connection-state via utility methods on SQLServerConnection, esp sp_setapprole since it faces 'special' issues. [FEATURE REQUEST] Provide a way to set SQLServer-specific connection-state via utility methods on SQLServerConnection, esp sp_setapprole since it faces 'special' issues. Mar 17, 2021
@c-m-d-1234
Copy link

the problem with doing the following:-
EXEC @result = sys.sp_unsetapprole @cookie = 0x00...FF;
is every statement has to be compiled as the cookie is different every time, with our server that cause a sever problem with cache locks on compilation and brought the server to it knees so we had to go back to using bind variable and using JTDS driver. I can not see why the Microsoft JDBC driver can not do the same thing as the JTDS driver or have a option to stop it doing the sp_prepexec.

@dtbullock
Copy link
Author

dtbullock commented Jun 10, 2021

Hi @c-m-d-1234 I had a hard time understanding why you were able to get a parameterized/compiled version of statements which used sp_*approle with JTDS.

That would only make sense if sp_*approle was OK with being invoked via sp_prepare (and subsequently, sp_execute) despite refusing to work when invoked with sp_prepexec. But this may be the case ... it is certainly suggested by the 'trace' shown here (although the article is about something else). I have yet to test it.

Probably the MS JDBC driver doesn't want to make it a general option to choose between the sp_prepare and sp_prepexec strategies on a per-statement basis. (Though if they did this and also allowed a sp_executesql strategy as well, they might win over some critics). This would add complexity/bulk and isn't a likely direction. Nonetheless, being able to use sp_prepare 'behind the scenes' of an API concerned with setting/unsetting the app_role would be an implementation opportunity.

@dtbullock
Copy link
Author

FWIW, I have asked the SQL Server team to permit sp_setapprole under sp_prepexec:

@Jeffery-Wasty Jeffery-Wasty added the Backlog The topic in question has been recognized and added to development backlog label Mar 10, 2022
@github-project-automation github-project-automation bot moved this to To be triaged in MSSQL JDBC Aug 28, 2024
@Jeffery-Wasty Jeffery-Wasty moved this from To be triaged to Backlog in MSSQL JDBC Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backlog The topic in question has been recognized and added to development backlog Enhancement An enhancement to the driver. Lower priority than bugs.
Projects
Status: Backlog
Development

No branches or pull requests

4 participants