CurtHagenlocher commented on code in PR #2822: URL: https://github.com/apache/arrow-adbc/pull/2822#discussion_r2087017443
########## csharp/src/Drivers/Apache/Thrift/Sasl/TSaslTransport.cs: ########## @@ -0,0 +1,181 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more +* contributor license agreements. See the NOTICE file distributed with +* this work for additional information regarding copyright ownership. +* The ASF licenses this file to You under the Apache License, Version 2.0 +* (the "License"); you may not use this file except in compliance with +* the License. You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ + +using System; +using System.Security.Authentication; +using System.Text; +using System.Threading.Tasks; +using System.Threading; +using Thrift; +using Thrift.Transport; + +namespace Apache.Arrow.Adbc.Drivers.Apache.Thrift.Sasl +{ + public class TSaslTransport : TEndpointTransport + { + private readonly TTransport _innerTransport; + private readonly ISaslMechanism _saslMechanism; + + protected const int MECHANISM_NAME_BYTES = 1; + protected const int STATUS_BYTES = 1; + protected const int PAYLOAD_LENGTH_BYTES = 4; + + private byte[] messageHeader = new byte[STATUS_BYTES + PAYLOAD_LENGTH_BYTES]; + + public TSaslTransport(TTransport innerTransport, ISaslMechanism saslMechanism, TConfiguration config) + : base(config) + { + _innerTransport = innerTransport ?? throw new ArgumentNullException(nameof(innerTransport)); + _saslMechanism = saslMechanism ?? throw new ArgumentNullException(nameof(saslMechanism)); + } + + public override bool IsOpen => _innerTransport.IsOpen; + + public override async Task OpenAsync(CancellationToken cancellationToken = default) + { + // Open the transport and negotiate SASL authentication + await _innerTransport.OpenAsync(cancellationToken).ConfigureAwait(false); + await NegotiateAsync(cancellationToken).ConfigureAwait(false); + } + + public override void Close() + { + _innerTransport.Close(); + } + + public override ValueTask<int> ReadAsync(byte[] buffer, int offset, int length, CancellationToken cancellationToken = default) + { + return _innerTransport.ReadAsync(buffer, offset, length, cancellationToken); + } + + public override Task WriteAsync(byte[] buffer, int offset, int length, CancellationToken cancellationToken = default) + { + return _innerTransport.WriteAsync(buffer, offset, length, cancellationToken); + } + + public override Task FlushAsync(CancellationToken cancellationToken = default) + { + return _innerTransport.FlushAsync(cancellationToken); + } + + protected override void Dispose(bool disposing) + { + if (disposing) + { + _innerTransport.Dispose(); + } + } + + private async Task NegotiateAsync(CancellationToken cancellationToken) + { + // Send the SASL mechanism name + await SendMechanismAsync(_saslMechanism.Name, cancellationToken).ConfigureAwait(false); + + // Send the authentication message + var authMessage = _saslMechanism.EvaluateChallenge(null); + await SendSaslMessageAsync(NegotiationStatus.Ok, authMessage, cancellationToken).ConfigureAwait(false); + + // Receive server's response (authentication status) + var serverResponse = await ReceiveSaslMessageAsync(cancellationToken).ConfigureAwait(false); + + if (serverResponse.status == null || serverResponse.status != NegotiationStatus.Complete) + { + throw new AuthenticationException($"SASL {_saslMechanism.Name} authentication failed."); + } + + _saslMechanism.IsNegotiationCompleted = true; + } + + private async Task SendMechanismAsync(string mechanismName, CancellationToken cancellationToken) + { + // Send the mechanism name to the server + byte[] mechanismNameBytes = Encoding.UTF8.GetBytes(mechanismName); + await SendSaslMessageAsync(NegotiationStatus.Start, mechanismNameBytes, cancellationToken); + } + + + protected async Task SendSaslMessageAsync(NegotiationStatus status, byte[] payload, CancellationToken cancellationToken) + { + if (payload == null) payload = new byte[0]; + // Set status byte + messageHeader[0] = status.Value; + // Encode payload length + EncodeBigEndian(payload.Length, messageHeader, STATUS_BYTES); Review Comment: Consider using `BinaryPrimitives.WriteInt32BigEndian` and `BinaryPrimitives.ReadInt32BigEndian` instead of writing the implementation manually. ########## csharp/src/Drivers/Apache/Thrift/Sasl/PlainSaslMechanism.cs: ########## @@ -0,0 +1,65 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more +* contributor license agreements. See the NOTICE file distributed with +* this work for additional information regarding copyright ownership. +* The ASF licenses this file to You under the Apache License, Version 2.0 +* (the "License"); you may not use this file except in compliance with +* the License. You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ + +using System; +using System.Text; + +namespace Apache.Arrow.Adbc.Drivers.Apache.Thrift.Sasl; Review Comment: We should be consistent about namespace formatting (declaration vs scope). ########## csharp/src/Drivers/Apache/Thrift/Sasl/ISaslMechanism.cs: ########## @@ -0,0 +1,43 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more +* contributor license agreements. See the NOTICE file distributed with +* this work for additional information regarding copyright ownership. +* The ASF licenses this file to You under the Apache License, Version 2.0 +* (the "License"); you may not use this file except in compliance with +* the License. You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ + +namespace Apache.Arrow.Adbc.Drivers.Apache.Thrift.Sasl +{ + /// <summary> + /// Defines the contract for implementing SASL authentication mechanisms (e.g., PLAIN, GSSAPI). + /// This interface allows the client to authenticate over a Thrift transport using the selected SASL mechanism. + /// </summary> + public interface ISaslMechanism Review Comment: Is there a reason to make the SASL types public instead of internal? ########## csharp/src/Drivers/Apache/Thrift/Sasl/TSaslTransport.cs: ########## @@ -0,0 +1,181 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more +* contributor license agreements. See the NOTICE file distributed with +* this work for additional information regarding copyright ownership. +* The ASF licenses this file to You under the Apache License, Version 2.0 +* (the "License"); you may not use this file except in compliance with +* the License. You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ + +using System; +using System.Security.Authentication; +using System.Text; +using System.Threading.Tasks; +using System.Threading; +using Thrift; +using Thrift.Transport; + +namespace Apache.Arrow.Adbc.Drivers.Apache.Thrift.Sasl +{ + public class TSaslTransport : TEndpointTransport + { + private readonly TTransport _innerTransport; + private readonly ISaslMechanism _saslMechanism; + + protected const int MECHANISM_NAME_BYTES = 1; + protected const int STATUS_BYTES = 1; + protected const int PAYLOAD_LENGTH_BYTES = 4; + + private byte[] messageHeader = new byte[STATUS_BYTES + PAYLOAD_LENGTH_BYTES]; + + public TSaslTransport(TTransport innerTransport, ISaslMechanism saslMechanism, TConfiguration config) + : base(config) + { + _innerTransport = innerTransport ?? throw new ArgumentNullException(nameof(innerTransport)); + _saslMechanism = saslMechanism ?? throw new ArgumentNullException(nameof(saslMechanism)); + } + + public override bool IsOpen => _innerTransport.IsOpen; + + public override async Task OpenAsync(CancellationToken cancellationToken = default) + { + // Open the transport and negotiate SASL authentication + await _innerTransport.OpenAsync(cancellationToken).ConfigureAwait(false); + await NegotiateAsync(cancellationToken).ConfigureAwait(false); + } + + public override void Close() + { + _innerTransport.Close(); + } + + public override ValueTask<int> ReadAsync(byte[] buffer, int offset, int length, CancellationToken cancellationToken = default) + { + return _innerTransport.ReadAsync(buffer, offset, length, cancellationToken); + } + + public override Task WriteAsync(byte[] buffer, int offset, int length, CancellationToken cancellationToken = default) + { + return _innerTransport.WriteAsync(buffer, offset, length, cancellationToken); + } + + public override Task FlushAsync(CancellationToken cancellationToken = default) + { + return _innerTransport.FlushAsync(cancellationToken); + } + + protected override void Dispose(bool disposing) + { + if (disposing) + { + _innerTransport.Dispose(); + } + } + + private async Task NegotiateAsync(CancellationToken cancellationToken) + { + // Send the SASL mechanism name + await SendMechanismAsync(_saslMechanism.Name, cancellationToken).ConfigureAwait(false); + + // Send the authentication message + var authMessage = _saslMechanism.EvaluateChallenge(null); + await SendSaslMessageAsync(NegotiationStatus.Ok, authMessage, cancellationToken).ConfigureAwait(false); + + // Receive server's response (authentication status) + var serverResponse = await ReceiveSaslMessageAsync(cancellationToken).ConfigureAwait(false); + + if (serverResponse.status == null || serverResponse.status != NegotiationStatus.Complete) + { + throw new AuthenticationException($"SASL {_saslMechanism.Name} authentication failed."); + } + + _saslMechanism.IsNegotiationCompleted = true; + } + + private async Task SendMechanismAsync(string mechanismName, CancellationToken cancellationToken) + { + // Send the mechanism name to the server + byte[] mechanismNameBytes = Encoding.UTF8.GetBytes(mechanismName); + await SendSaslMessageAsync(NegotiationStatus.Start, mechanismNameBytes, cancellationToken); + } + + Review Comment: nit: extra blank line ########## csharp/src/Drivers/Apache/Thrift/Sasl/TSaslTransport.cs: ########## @@ -0,0 +1,181 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more +* contributor license agreements. See the NOTICE file distributed with +* this work for additional information regarding copyright ownership. +* The ASF licenses this file to You under the Apache License, Version 2.0 +* (the "License"); you may not use this file except in compliance with +* the License. You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ + +using System; +using System.Security.Authentication; +using System.Text; +using System.Threading.Tasks; +using System.Threading; +using Thrift; +using Thrift.Transport; + +namespace Apache.Arrow.Adbc.Drivers.Apache.Thrift.Sasl +{ + public class TSaslTransport : TEndpointTransport + { + private readonly TTransport _innerTransport; + private readonly ISaslMechanism _saslMechanism; + + protected const int MECHANISM_NAME_BYTES = 1; + protected const int STATUS_BYTES = 1; + protected const int PAYLOAD_LENGTH_BYTES = 4; + + private byte[] messageHeader = new byte[STATUS_BYTES + PAYLOAD_LENGTH_BYTES]; + + public TSaslTransport(TTransport innerTransport, ISaslMechanism saslMechanism, TConfiguration config) + : base(config) + { + _innerTransport = innerTransport ?? throw new ArgumentNullException(nameof(innerTransport)); + _saslMechanism = saslMechanism ?? throw new ArgumentNullException(nameof(saslMechanism)); + } + + public override bool IsOpen => _innerTransport.IsOpen; + + public override async Task OpenAsync(CancellationToken cancellationToken = default) + { + // Open the transport and negotiate SASL authentication + await _innerTransport.OpenAsync(cancellationToken).ConfigureAwait(false); + await NegotiateAsync(cancellationToken).ConfigureAwait(false); + } + + public override void Close() + { + _innerTransport.Close(); + } + + public override ValueTask<int> ReadAsync(byte[] buffer, int offset, int length, CancellationToken cancellationToken = default) + { + return _innerTransport.ReadAsync(buffer, offset, length, cancellationToken); + } + + public override Task WriteAsync(byte[] buffer, int offset, int length, CancellationToken cancellationToken = default) + { + return _innerTransport.WriteAsync(buffer, offset, length, cancellationToken); + } + + public override Task FlushAsync(CancellationToken cancellationToken = default) + { + return _innerTransport.FlushAsync(cancellationToken); + } + + protected override void Dispose(bool disposing) + { + if (disposing) + { + _innerTransport.Dispose(); + } + } + + private async Task NegotiateAsync(CancellationToken cancellationToken) + { + // Send the SASL mechanism name + await SendMechanismAsync(_saslMechanism.Name, cancellationToken).ConfigureAwait(false); + + // Send the authentication message + var authMessage = _saslMechanism.EvaluateChallenge(null); + await SendSaslMessageAsync(NegotiationStatus.Ok, authMessage, cancellationToken).ConfigureAwait(false); + + // Receive server's response (authentication status) + var serverResponse = await ReceiveSaslMessageAsync(cancellationToken).ConfigureAwait(false); + + if (serverResponse.status == null || serverResponse.status != NegotiationStatus.Complete) + { + throw new AuthenticationException($"SASL {_saslMechanism.Name} authentication failed."); + } + + _saslMechanism.IsNegotiationCompleted = true; + } + + private async Task SendMechanismAsync(string mechanismName, CancellationToken cancellationToken) + { + // Send the mechanism name to the server + byte[] mechanismNameBytes = Encoding.UTF8.GetBytes(mechanismName); + await SendSaslMessageAsync(NegotiationStatus.Start, mechanismNameBytes, cancellationToken); + } + + + protected async Task SendSaslMessageAsync(NegotiationStatus status, byte[] payload, CancellationToken cancellationToken) + { + if (payload == null) payload = new byte[0]; Review Comment: This shouldn't be necessary because it's not a public API and `payload` is not marked as nullable, but this can be done more efficiently with either `Array.Empty<char>()` or the literal `[]` (which gets translated into Array.Empty) as they won't require an allocation. ########## csharp/src/Drivers/Apache/Thrift/Sasl/TSaslTransport.cs: ########## @@ -0,0 +1,181 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more +* contributor license agreements. See the NOTICE file distributed with +* this work for additional information regarding copyright ownership. +* The ASF licenses this file to You under the Apache License, Version 2.0 +* (the "License"); you may not use this file except in compliance with +* the License. You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ + +using System; +using System.Security.Authentication; +using System.Text; +using System.Threading.Tasks; +using System.Threading; +using Thrift; +using Thrift.Transport; + +namespace Apache.Arrow.Adbc.Drivers.Apache.Thrift.Sasl +{ + public class TSaslTransport : TEndpointTransport + { + private readonly TTransport _innerTransport; + private readonly ISaslMechanism _saslMechanism; + + protected const int MECHANISM_NAME_BYTES = 1; + protected const int STATUS_BYTES = 1; + protected const int PAYLOAD_LENGTH_BYTES = 4; + + private byte[] messageHeader = new byte[STATUS_BYTES + PAYLOAD_LENGTH_BYTES]; + + public TSaslTransport(TTransport innerTransport, ISaslMechanism saslMechanism, TConfiguration config) + : base(config) + { + _innerTransport = innerTransport ?? throw new ArgumentNullException(nameof(innerTransport)); + _saslMechanism = saslMechanism ?? throw new ArgumentNullException(nameof(saslMechanism)); + } + + public override bool IsOpen => _innerTransport.IsOpen; + + public override async Task OpenAsync(CancellationToken cancellationToken = default) + { + // Open the transport and negotiate SASL authentication + await _innerTransport.OpenAsync(cancellationToken).ConfigureAwait(false); + await NegotiateAsync(cancellationToken).ConfigureAwait(false); + } + + public override void Close() + { + _innerTransport.Close(); + } + + public override ValueTask<int> ReadAsync(byte[] buffer, int offset, int length, CancellationToken cancellationToken = default) + { + return _innerTransport.ReadAsync(buffer, offset, length, cancellationToken); + } + + public override Task WriteAsync(byte[] buffer, int offset, int length, CancellationToken cancellationToken = default) + { + return _innerTransport.WriteAsync(buffer, offset, length, cancellationToken); + } + + public override Task FlushAsync(CancellationToken cancellationToken = default) + { + return _innerTransport.FlushAsync(cancellationToken); + } + + protected override void Dispose(bool disposing) + { + if (disposing) + { + _innerTransport.Dispose(); + } + } + + private async Task NegotiateAsync(CancellationToken cancellationToken) + { + // Send the SASL mechanism name + await SendMechanismAsync(_saslMechanism.Name, cancellationToken).ConfigureAwait(false); + + // Send the authentication message + var authMessage = _saslMechanism.EvaluateChallenge(null); + await SendSaslMessageAsync(NegotiationStatus.Ok, authMessage, cancellationToken).ConfigureAwait(false); + + // Receive server's response (authentication status) + var serverResponse = await ReceiveSaslMessageAsync(cancellationToken).ConfigureAwait(false); + + if (serverResponse.status == null || serverResponse.status != NegotiationStatus.Complete) + { + throw new AuthenticationException($"SASL {_saslMechanism.Name} authentication failed."); + } + + _saslMechanism.IsNegotiationCompleted = true; + } + + private async Task SendMechanismAsync(string mechanismName, CancellationToken cancellationToken) + { + // Send the mechanism name to the server + byte[] mechanismNameBytes = Encoding.UTF8.GetBytes(mechanismName); + await SendSaslMessageAsync(NegotiationStatus.Start, mechanismNameBytes, cancellationToken); + } + + + protected async Task SendSaslMessageAsync(NegotiationStatus status, byte[] payload, CancellationToken cancellationToken) + { + if (payload == null) payload = new byte[0]; + // Set status byte + messageHeader[0] = status.Value; + // Encode payload length + EncodeBigEndian(payload.Length, messageHeader, STATUS_BYTES); + await _innerTransport.WriteAsync(messageHeader, 0, messageHeader.Length); + await _innerTransport.WriteAsync(payload, 0, payload.Length); + await _innerTransport.FlushAsync(cancellationToken); + } + + protected async Task<SaslResponse> ReceiveSaslMessageAsync(CancellationToken cancellationToken = default) + { + await _innerTransport.ReadAllAsync(messageHeader, 0, messageHeader.Length, cancellationToken); + + byte statusByte = messageHeader[0]; + + NegotiationStatus? status = NegotiationStatus.FromValue(statusByte); + if (status == null) + { + throw new TTransportException("Received invalid SASL negotiation status. The status byte was null."); + } + + int payloadBytes = DecodeBigEndian(messageHeader, STATUS_BYTES); + if (payloadBytes < 0 || payloadBytes > new TConfiguration().MaxMessageSize) Review Comment: Instead of creating a new `TConfiguration` here, shouldn't it use the value passed into the constructor? ########## csharp/src/Drivers/Apache/Thrift/Sasl/PlainSaslMechanism.cs: ########## @@ -0,0 +1,65 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more +* contributor license agreements. See the NOTICE file distributed with +* this work for additional information regarding copyright ownership. +* The ASF licenses this file to You under the Apache License, Version 2.0 +* (the "License"); you may not use this file except in compliance with +* the License. You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ + +using System; +using System.Text; + +namespace Apache.Arrow.Adbc.Drivers.Apache.Thrift.Sasl; + +/// <summary> +/// Implements the SASL PLAIN mechanism for simple username/password authentication. +/// </summary> +public class PlainSaslMechanism : ISaslMechanism +{ + private readonly string _username; + private readonly string _password; + private readonly string _authorizationId; + private bool _isNegotiationCompleted; + + /// <summary> + /// Initializes a new instance of the <see cref="PlainSaslMechanism"/> class. + /// </summary> + /// <param name="username">The username for authentication.</param> + /// <param name="password">The password for authentication.</param> + public PlainSaslMechanism(string username, string password, string authorizationId = "") + { + _username = username ?? throw new ArgumentNullException(nameof(username)); + _password = password ?? throw new ArgumentNullException(nameof(password)); + _authorizationId = authorizationId; + } + + public string Name => "PLAIN"; + + public byte[] EvaluateChallenge(byte[]? challenge) + { + if (_isNegotiationCompleted) + { + // PLAIN is single-step, so return empty array if already done + return new byte[0]; Review Comment: Again, either `Array.Empty<byte>()` or `[]` is more efficient. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
