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 Support For Secure Connection Bundle #1487

Closed
wants to merge 16 commits into from
Closed

Conversation

giltohar
Copy link

@giltohar giltohar commented Aug 31, 2020

There is no existing gocql driver support for connecting to Cassandra Databases via Secure Connection Bundle, which our team at IBM required. IBM Cassandra is switching to Secure Connection Bundles and this will be needed for anyone using this golang driver to connect to them. We've been using this gocql driver now for a long time to connect to our previous Cassandra instance, and made the updates required to add connectivity support for Cassandra databases which use the Secure Connection Bundle.

@giltohar
Copy link
Author

giltohar commented Sep 11, 2020

@Zariel @martin-sucha hi my team has been using this gocql driver for a few years now. Recently, we switched our database to a Datastax enterprise, which required us to make some updates to the initialization functionality to allow for connections to be made using secure connect bundles. We're using our forked version now and are interested in contributing it.

I was wondering if someone might be able to share some insight as to the issues in the build as it's not clear from the build logs what issue occurred, if that is a blocker in us getting this code merged in.

@giltohar
Copy link
Author

I noticed all the tests that failed had AUTH=false which likely has something to do with it

@giltohar
Copy link
Author

@Zariel @martin-sucha found the issue and fixed :)

@Zariel
Copy link
Contributor

Zariel commented Sep 12, 2020

Im not happy with adding more and more specialisation to the driver at this point, it is becoming bloated and difficult to maintain. If this can't be implemented outside of the tree then I would like to know why and fix the driver so that it can be.

@giltohar
Copy link
Author

@Zariel I'm not sure of any way how to do that. Basically we need to use SNI and not rely on each host having its own specific IPADDRESS, instead they are identified by their Cassdandra node DB host id (not related to connect ipaddress or dns host names). This requires changes in the client due to how it initializes connections

@giltohar
Copy link
Author

Hi. @Zariel Any thoughts? We think this is a good improvement to the driver, as it makes it compatible to work with more Cassandra instances, for example the Cassandra instance of the Datastax cassandra, which is a major provider of Cassandra.

@phact
Copy link

phact commented Oct 13, 2020

(Full disclosure, I work at DataStax)

Hi @Zariel thanks for all your work with this driver. Been using a bunch more c* and golang lately and it's been good to work with.

I wanted to point out that the SNI proxy functionality in this patch (thank you @giltohar!) is not DataStax Astra specific. As more and more folks use c* with k8s a desire to get at the database from a single endpoint is desirable and SNI is a nice way to achieve that and still maintain token awareness.

Here's how you would set up Cassandra with listio and SNI: https://www.solo.io/blog/step-by-step-datastax-cassandra-with-istio-and-sni-routing/

In general I think this patch is a nice addition given the changing cloud native landscape and a good way to help expand the reach of gocql to more users.

@giltohar
Copy link
Author

@Zariel any thoughts? :)

@martin-sucha
Copy link
Contributor

martin-sucha commented Oct 24, 2020

I think that it would be useful to have this functionality in this repository (as opposed to an external one). But we should add it in a way so that the implementation is isolated in one place, for example in a sub-package and could be theoretically implemented in an external package.

We currently have Dialer config option that establishes raw network connection to given TCP address, but the Dialer interface is not sufficient to implement TLS bundle. We need a more high level interface for "get a connection to given host" to be able to do that.

So what about an interface that would look similar to this (I don't have a better name than Dialer2 at the moment):

type Dialer2 interface {
	// Dial establishes new connection to host.
	// It is responsibility of Dial to setup TLS if necessary.
	Dial(ctx context.Context, host *HostInfo) (net.Conn, error)
}

Interface like Dialer2 could be used to replace current Dialer and implement TLS bundle.
If it was possible to construct custom HostInfo from outside of the package, this could also replace AddressTranslator interface, since AddressTranslator could be implemented as a wrapper for some other Dialer2:

type TranslatorDialer struct {
    dialer Dialer2
}

func (td *TranslatorDialer) Dial(ctx context.Context, host *HostInfo) (net.Conn, error) {
    newHost := host.Clone()
    newHost.HostName = "192.0.2.1"
    return td.dialer.Dial(ctx, newHost)
}


// add each file from bundle to the directory
for _, f := range r.File {
fpath := filepath.Join(dir, f.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Extracting a file like this is a security issue - if the zip was malicious, it could replace any file it wanted. The documentation for Name says:

    // It is the caller's responsibility to sanitize it as
    // appropriate, including canonicalizing slash directions,
    // validating that paths are relative, and preventing path
    // traversal through filenames ("../../../").

// b) 'key' file
// c) 'ca.crt' file

dir, err := ioutil.TempDir("", "securezip")
Copy link
Contributor

@martin-sucha martin-sucha Oct 24, 2020

Choose a reason for hiding this comment

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

We definitely shouldn't write to disk in gocql. It's a network client so nobody expects it will write to disk. We cannot even assume that it's possible to write to disk, that could be forbidden for example due to host app's security policy.

It is not necessary to write to disk to setup tls.Config either, we could just:

  1. Find entries in zip for cert, key and ca.crt and read the contents to memory
  2. Use tls.X509KeyPair to parse the PEM key pair and put the result to tlsConfig.Certificates
  3. Create new cert pool x509.NewCertPool() and add the CA certificate to it using it's AppendCertsFromPEM, put this to tlsConfig.RootCAs

}
defer resp.Body.Close()
if resp.StatusCode > 299 {
// err := utils.NewError(resp.StatusCode, msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably handle the case here.

@Zariel
Copy link
Contributor

Zariel commented Oct 24, 2020

My main concern is this touches so much of the codebase and that to me tells me the abstractions are wrong. On top of that passing in path to parse the data from is just plain wrong and should be accepted in a way the user does the setup work. Also I would rather more functionality is not added to the main gocql package where possible.

I think in general these sort of PR's could do with some design work and planning through issues.

@mpenick
Copy link
Contributor

mpenick commented Sep 13, 2021

We've build a proxy, called cql-proxy that allows gocql (or any other CQL driver) to work without this patch. It allows gocql applications to connect without any or very little modification.

@martin-sucha
Copy link
Contributor

Interesting, thank you for the link to the proxy! Today I learned that https://github.com/datastax/go-cassandra-native-protocol exists :)

I still think it would be useful to be be able to use the connection bundle with gocql directly. However, we need to use the correct abstractions to keep gocql maintainable. Something like the dialer based on *Host that would also handle TLS (mentioned above) and a custom function to retrieve the initial host list.

@mpenick Do you happen to have some simple server setup in docker-compose (or something similar) for testing on a local machine? Or would you be willing to contribute a patch? I'd be happy to review it.

@martin-sucha
Copy link
Contributor

#1629 implements interface similar to the one suggested in #1487 (comment), so it should now be possible to implement the secure connection bundle in an external package.

@martin-sucha
Copy link
Contributor

Closing this pull request as #1629 is now merged. It should be possible to implement this in an external package using the new HostDialer interface.

@mpenick
Copy link
Contributor

mpenick commented Aug 11, 2022

@martin-sucha Great work on the HostDialer interface. 🙌 ! I've made a small library that allows users to connect to Astra using that interface: https://github.com/mpenick/gocqlastra. It could be useful for your work too. Now I just need to verify that topology/status events work correctly.

@martin-sucha
Copy link
Contributor

@mpenick Nice! Feel free to add a link to the gocqlastra library to gocql's README.

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.

6 participants