Sessions module is insecure

[mjohnson9]

The sessions module is currently insecure due to its fixed key. This should probably be fixed. (It should more than likely be supplied with both keys so that it uses encryption, as noted in the documentation.)

You should probably also change it to use the new Gorilla repositories.

[kylefinley]

I agree, I'm still undecided on how to handle config settings, however. What do you think the best way supply the key would be?

You should probably also change it to use the new Gorilla repositories.

I've update the repositories under the gaego organization to the new Gorilla repositories

[mjohnson9]

I believe the best course of action would be:

1. Open a transaction
2. Attempt to get a config for SessionKeys
3. If those don't exist, randomly generate two keys, by the names AuthenticationKey and EncryptionKey
4. Store those under the SessionKeys config
5. Use AuthenticationKey and EncryptionKey to initialize the CookieStore

This will probably require a rewrite of the API for the sessions, though, as you currently don't seem to be able to do that at initialization.

What do you think of this?

[kylefinley]

That sound good. The difficult part -- I think -- is step #1 opening a transaction. Ideally we could just put this logic in the init function, but since all transactions require a request it will not work.

To circumvent this limitation I believe the options would be:

1. Create an inprocess config -- one that doesn't store to the datastore.
2. Pull values from a global config file in the init method -- the config settings would not be editable.
3. Devise a mechanism for setting default config settings with the first request to an instance. I though about adding logic to the 'gaego/context' that would copy settings from a config file to a 'gaego/config' with the first request.

What do you think? Is there a better way to handle this?

[mjohnson9]

I'm wondering if you actually have to have a valid context. If I remember correctly, the Python runtime is able to do datastore operations at instance startup. I'm going to mess around with some code and test it out. After that I'll come back and write the pros and cons of all of the options.

[mjohnson9]

It seems that the Go runtime uses the Context (in datastore operations, at least) for the current app's ID (ID is passed to the runtime via HTTP headers.) The Python protobuffer instances have an _app property, but I can't figure out where it's being set. In both instances the app ID seems to be required for a datastore RPC.

[mjohnson9]

First of all, I think I should clarify how I believe we would set it up to use a Config:

1. We would mock the Gorilla CookieStore interface
2. The first time any function that requires the session store is called, we would initialize the configuration (and by extension the cryptographic keys) using a sync.Once
3. In this session store initialization, we would do the steps that I described above. (If the initialization errored for whatever reason, we would reset the sync.Once by overwriting it with a new instance and return the error to the caller)
4. We would call the actual function on the newly-initialized (or not if it had been initialized previously) session store.

I don't believe that storing a hard-coded configuration is a very good idea at all for many reasons, primarily that it requires you to either use the same configuration for production and development or change it any time you wish to deploy.

[kylefinley]

That sounds good it solves that problem of need the request object. A few questions:

1. How would set the SessionKeys? Through an admin interface?
2. If setting the keys in an admin interface will there be a way to set default values for development and testing.

[mjohnson9]

1. How would set the SessionKeys? Through an admin interface?

I believe they should be randomly generated, but only if the Get of the Config fails.

[kylefinley]

Good point.

OK, I'm convenced. Do you mind making the change? You should have full write access to gaego/session. I really appreciate all of your contributions.

[mjohnson9]

Do you mind making the change?

I'll begin work on it today or tomorrow. :)

[mjohnson9]

I believe I've thought of an easier-to-maintain iteration of essentially the same idea.

What if, instead of mocking the entire sessions package, we made a function that returned an initialized session store? It seems to make more sense and still allows us to do all of the initialization easily.

[kylefinley]

Sure, that could work. Which ever you think would make the api easiest to use.

I think it's alright to be opinionated and only offer one type of store, by removing the configuration all we really need are the Get and Save functions.

import "github.com/gaego/session"

func MyHandler(w http.ResponseWriter, r *http.Request) {
    // Get a session and set a value.
    sess1, _ := session.Get(r, "session-one")
    sess1.Values["foo"] = "bar"
    // Get another session and set another value.
    sess2, _ := session.Get(r, "session-two")
    sess2.Values[42] = 43
    // Save all sessions.
    session.Save(r, w)
}

I apologize if I'm overlooking looking something, it's been a while since I've thought about sessions.

[mjohnson9]

I was thinking more of:

import "github.com/gaego/session"

func MyHandler(w http.ResponseWriter, r *http.Request) {
    // Get a store (this initializes the store keys and/or store if necessary)
    store := session.GetStore(w, r)

    // Get a session and set a value.
    sess1, _ := store.Get(r, "session-one")
    sess1.Values["foo"] = "bar"
    // Get another session and set another value.
    sess2, _ := store.Get(r, "session-two")
    sess2.Values[42] = 43
    // Save all sessions.
    store.Save(r, w)
}

[kylefinley]

Looks good to me :)

[mjohnson9]

I've created a branch named secure where I'm beginning work.