openshift / origin

Conformance test suite for OpenShift
http://www.openshift.org
Apache License 2.0
8.48k stars 4.7k forks source link

/exec endpoint with base64.channel.k8s.io does not decode data #4985

Closed stefwalter closed 8 years ago

stefwalter commented 8 years ago

The /exec WebSocket pod endpoint using the "base64.channel.k8s.io" protocol does not properly decode data.

If I open a WebSocket to .../exec?command=/bin/sh&... and ws.send("0", btoa("test")) I can see base64 characters being echoed.

stefwalter commented 8 years ago

Noted in upstream review https://github.com/kubernetes/kubernetes/pull/13885

stefwalter commented 8 years ago

CC @smarterclayton @jwforres

smarterclayton commented 8 years ago

Hrm, this is how the protocol is designed (it only encodes on the way back) since it's easy for browsers to send raw text. Do you need stdin to be base46 encoded for a reason?

stefwalter commented 8 years ago

Hrm, this is how the protocol is designed (it only encodes on the way back) since it's easy for browsers to send raw text.

I see. Well not a big deal.

But the assymetry is rather ugly. The /exec protocols have 3 kinds of framing between them:

Do you need stdin to be base46 encoded for a reason?

Well, I don't need it for present use cases. But I can think of a situation: If you want to pipe binary data from the browser to a command running in a container, while displaying resulting output from that command easily (ie: in base64 mode)

smarterclayton commented 8 years ago

When you get bytes from an uploaded binary file in the browser can you get it in byte buffer or string form? I thought that was possible today.

On Oct 7, 2015, at 9:02 AM, Stef Walter notifications@github.com wrote:

Hrm, this is how the protocol is designed (it only encodes on the way back) since it's easy for browsers to send raw text.

I see. Well not a big deal.

But the assymetry is rather ugly. The /exec protocols have 3 kinds of framing between them:

Do you need stdin to be base46 encoded for a reason?

Well, I don't need it for present use cases. But I can think of a situation: If you want to pipe binary data from the browser to a command running in a container, while displaying resulting output from that command easily (ie: in base64 mode)

— Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/4985#issuecomment-146189983.

stefwalter commented 8 years ago

When you get bytes from an uploaded binary file in the browser can you get it in byte buffer or string form? I thought that was possible today.

Everything is possible ... including UTF8 decoding from ArrayBuffer... (ie: the whole point of base64 encoding at all). My comment was about utility and protocol symmetry.

As I said this is not a blocker. If this it's the way you intended it to be, feel free to close the pull request.

smarterclayton commented 8 years ago

Basically for the binary channel I implemented what I believed was the bare minimum to be useful in the browser. If we can't easily move byte buffer data from a binary source in the browser into STDIN i would agree we would need the decoding.

On Wed, Oct 7, 2015 at 9:26 AM, Clayton Coleman ccoleman@redhat.com wrote:

When you get bytes from an uploaded binary file in the browser can you get it in byte buffer or string form? I thought that was possible today.

On Oct 7, 2015, at 9:02 AM, Stef Walter notifications@github.com wrote:

Hrm, this is how the protocol is designed (it only encodes on the way back) since it's easy for browsers to send raw text.

I see. Well not a big deal.

But the assymetry is rather ugly. The /exec protocols have 3 kinds of framing between them:

  • channel.k8s.io: 0-3 +
  • base64.channel.k8s.io: '0'-'3' +
  • base64.channel.k8s.io: '0'-'3' + base64()

Do you need stdin to be base46 encoded for a reason?

Well, I don't need it for present use cases. But I can think of a situation: If you want to pipe binary data from the browser to a command running in a container, while displaying resulting output from that command easily (ie: in base64 mode)

— Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/4985#issuecomment-146189983.

stefwalter commented 8 years ago

Basically for the binary channel I implemented what I believed was the bare minimum to be useful in the browser. If we can't easily move byte buffer data from a binary source in the browser into STDIN i would agree we would need the decoding.

I think it's possible without the base64 decoding. One would:

  1. Use FileReader.readAsArrayBuffer.
  2. Create new ArrayBuffer one byte larger.
  3. Add '0' to the front of each ArrayBuffer block, and copy the data in.
  4. Send each ArrayBuffer down the pipe.

At the end of the day, it's all possible as is.

BTW, if we're going to have protocol asymmetry anyway, then why have a '0' at the front of the data being sent from browser to client? It just complicates things. Without that byte, you could read the file as a blob, and pass the blob to the WebSocket directly.

smarterclayton commented 8 years ago

Hrm... that's a good point. Originally, I implemented the simplest possible channel scheme. That's a good argument that it should be symmetric - to avoid explaining to users. There's nothing special about a particular channel at the lower level, it's just data sent to the server doesn't have to be encoded but data coming back does.

On Wed, Oct 7, 2015 at 10:05 AM, Stef Walter notifications@github.com wrote:

Basically for the binary channel I implemented what I believed was the bare minimum to be useful in the browser. If we can't easily move byte buffer data from a binary source in the browser into STDIN i would agree we would need the decoding.

I think it's possible without the base64 decoding. One would:

  1. Use FileReader.readAsArrayBuffer.
  2. Create new ArrayBuffer one byte larger.
  3. Add '0' to the front of each ArrayBuffer block, and copy the data in.
  4. Send each ArrayBuffer down the pipe.

At the end of the day, it's all possible as is.

BTW, if we're going to have protocol asymmetry anyway, then why have a '0' at the front of the data being sent from browser to client? It just complicates things. Without that byte, you could read the file as a blob, and pass the blob to the WebSocket directly.

— Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/4985#issuecomment-146205041.

scher200 commented 5 years ago

@stefwalter could you maybe post your script to send data to stdin of the container with nodejs. I just can't get it to work:

// load std kubernetes nodejs library
const k8s = require('@kubernetes/client-node');
var kc = new k8s.KubeConfig();
kc.loadFromDefault();

var input = `line 1
`;
var inputBuffer = Buffer.alloc( input.length + 1 );
inputBuffer.writeInt8( 0, 0 );
if ( input instanceof Buffer ) {
    input.copy( inputBuffer, 1 );
} else {
    inputBuffer.write( input, 1 );
}
console.log( "prepared InputBuffer: ", inputBuffer );

var apiUri = "mycluster.com:6443";
var namespace = "default";
var pod = "busycat1-234234f6f-am5kv";

var https = require('https');

var opts = https.RequestOptions = {};
kc.applytoHTTPSOptions( opts );
var optsObj = {
    ca: opts.ca.toString(),
    key: opts.key.toString(),
    cert: opts.cert.toString(),
    tlsOptions: { rejectUnauthorized: false }
};

const WebSocket = require('ws');

var ws = {};
function exec ( cmd, callback ) {

    var response = '', uri = '';

    uri += `wss://${apiUri}/api/v1/namespaces/${namespace}/pods/${pod}/exec?`;
    uri += 'stdout=1&stdin=1&stderr=1';
    cmd.forEach( subCmd => uri += `&command=${encodeURIComponent(subCmd)}` );

    ws = new WebSocket(uri, "base64.channel.k8s.io", optsObj);

    ws.on('open', function open() {
        console.log( "connection opened" );
        setTimeout( () => {
            send();
        }, 500 );
    });

    ws.on(  'message', ( data ) => {
        if (data[0].match(/^[0-3]$/)) {
            response += Buffer.from( data.slice( 1 ), 'base64' ).toString( "ascii" );
        }
    }  );

    ws.on(  'close', () => callback(response)  );

    ws.on(  'error', ( error ) => {
        console.log( 'WS Error: ' + error.toString() );
    }  );

}

function send () {

    // var msg = inputBuffer.toString( "base64" );
    // var msg = Buffer.from(input).toString( "base64" );
    // var msg = "hoi\r\n";
    // var msg = new Buffer(input).toString( "base64" );
    var msg = "0"+input;
    console.log( "send: ", msg );
    ws.send(  msg, ( err ) => {

        if ( err !== null && err !== undefined ) {
            console.log( "error while sending: ", err );
        } else {
            console.log( "SendSUCCES" );
        }

    }  )
    setTimeout( () => {
        ws.close();
    }, 1000 );

}
exec(
    ['/bin/cat', '-'],
    ( response ) => {
        console.log( "connection closed" );
        console.log( "response: ", response );
    }
);
stefwalter commented 5 years ago

@scher200 The script ended up here: https://github.com/cockpit-project/cockpit/blob/master/pkg/kubernetes/scripts/kube-client-cockpit.js#L463

But it's very Cockpit specific.

scher200 commented 5 years ago

@stefwalter thanks, gonna try learn from that