Skip to content

Commit

Permalink
Fix escaping behavior and add tests around it (elastic#621)
Browse files Browse the repository at this point in the history
This fixes elastic#618.

This depends on elastic/logstash#7687 to actually run. Until that is merged and released integration tests here will fail. PR review should be done manually against that branch for both the INTEGRATION and SECURE_INTEGRATION branches for now.

This branch fixes escaping for the user parameter and also adds integration tests for secure scenarios, where user/password are used, including with odd escaped values. This all tests SSL to boot.
  • Loading branch information
andrewvc committed Jul 18, 2017
1 parent 09c3b52 commit e1b120d
Show file tree
Hide file tree
Showing 12 changed files with 219 additions and 69 deletions.
3 changes: 3 additions & 0 deletions .travis.yml
Expand Up @@ -8,6 +8,8 @@ env:
- INTEGRATION=true ES_VERSION=2.4.4 TEST_DEBUG=true
- INTEGRATION=true ES_VERSION=5.2.1 TEST_DEBUG=true
- INTEGRATION=true ES_VERSION=master TEST_DEBUG=true
- SECURE_INTEGRATION=true INTEGRATION=true ES_VERSION=5.5.0 TEST_DEBUG=true
- SECURE_INTEGRATION=true INTEGRATION=true ES_VERSION=master TEST_DEBUG=true
rvm:
- jruby-1.7.25
matrix:
Expand All @@ -18,6 +20,7 @@ matrix:
env: LOGSTASH_BRANCH=5.x
allow_failures:
- env: INTEGRATION=true ES_VERSION=master TEST_DEBUG=true
- env: SECURE_INTEGRATION=true INTEGRATION=true ES_VERSION=master TEST_DEBUG=true
fast_finish: true
install: true
script: ci/build.sh
Expand Down
57 changes: 51 additions & 6 deletions ci/run.sh
Expand Up @@ -27,19 +27,59 @@ setup_es() {
mkdir -p elasticsearch/config/scripts
cp $BUILD_DIR/spec/fixtures/scripts/groovy/* elasticsearch/config/scripts
cp $BUILD_DIR/spec/fixtures/scripts/painless/* elasticsearch/config/scripts

# If we're running with xpack SSL/Users enabled...
if [[ "$SECURE_INTEGRATION" == "true" ]]; then
yes y | elasticsearch/bin/elasticsearch-plugin install x-pack

es_yml=elasticsearch/config/elasticsearch.yml
cp -rv $BUILD_DIR/spec/fixtures/test_certs elasticsearch/config/test_certs
echo "xpack.security.http.ssl.enabled: true" >> $es_yml
echo "xpack.ssl.key: $BUILD_DIR/elasticsearch/config/test_certs/test.key" >> $es_yml
echo "xpack.ssl.certificate: $BUILD_DIR/elasticsearch/config/test_certs/test.crt" >> $es_yml
echo "xpack.ssl.certificate_authorities: [ '$BUILD_DIR/elasticsearch/config/test_certs/ca/ca.crt' ]" >> $es_yml
fi
}

start_es() {
es_args=$@
elasticsearch/bin/elasticsearch $es_args > /tmp/elasticsearch.log 2>/dev/null &
count=120
echo "Waiting for elasticsearch to respond..."
while ! curl --silent localhost:9200 && [[ $count -ne 0 ]]; do
es_url="http://localhost:9200"
if [[ "$SECURE_INTEGRATION" == "true" ]]; then
es_url="https://localhost:9200 -k"
fi

while ! curl --silent $es_url && [[ $count -ne 0 ]]; do
count=$(( $count - 1 ))
[[ $count -eq 0 ]] && return 1
sleep 1
done
echo "Elasticsearch is Up !"

if [[ "$SECURE_INTEGRATION" == "true" ]]; then
curl -XPUT -k 'https://elastic:changeme@localhost:9200/_xpack/security/user/elastic/_password' -H "Content-Type: application/json" -d '{
"password" : "testpass"
}'

# These are the two users used for secure integration tests
curl -XPOST -k 'https://elastic:testpass@localhost:9200/_xpack/security/user/simpleuser' -H "Content-Type: application/json" -d '{
"password" : "abc123",
"full_name" : "John Doe",
"email" : "john.doe@anony.mous",
"roles" : [ "superuser" ]
}'

curl -XPOST -k 'https://elastic:testpass@localhost:9200/_xpack/security/user/f%40ncyuser' -H "Content-Type: application/json" -d '{
"password" : "ab%12#",
"full_name" : "John Doe",
"email" : "john.doe@anony.mous",
"roles" : [ "superuser" ]
}'

fi

return 0
}

Expand Down Expand Up @@ -74,38 +114,43 @@ start_nginx() {

bundle install
if [[ "$INTEGRATION" != "true" ]]; then
bundle exec rspec -fd spec
bundle exec rspec -fd spec -t ~integration -t ~secure_integration
else
if [[ "$1" -eq "" ]]; then
spec_path="spec"
else
spec_path="$1"
fi

extra_tag_args="--tag ~secure_integration --tag integration"
if [[ "$SECURE_INTEGRATION" == "true" ]]; then
extra_tag_args="--tag secure_integration"
fi

case "$ES_VERSION" in
5.*)
setup_es https://artifacts.elastic.co/downloads/elasticsearch/elasticsearch-${ES_VERSION}.tar.gz
start_es -Escript.inline=true -Escript.stored=true -Escript.file=true
# Run all tests which are for versions > 5 but don't run ones tagged < 5.x. Include ingest, new template
bundle exec rspec -fd --tag ~secure_integration --tag integration --tag version_greater_than_equal_to_5x --tag update_tests:painless --tag update_tests:groovy --tag ~version_less_than_5x $spec_path
bundle exec rspec -fd $extra_tag_args --tag version_greater_than_equal_to_5x --tag update_tests:painless --tag update_tests:groovy --tag ~version_less_than_5x $spec_path
;;
2.*)
setup_es https://download.elastic.co/elasticsearch/elasticsearch/elasticsearch-$ES_VERSION.tar.gz
start_es -Des.script.inline=on -Des.script.indexed=on -Des.script.file=on
# Run all tests which are for versions < 5 but don't run ones tagged 5.x and above. Skip ingest, new template
bundle exec rspec -fd --tag ~secure_integration --tag integration --tag version_less_than_5x --tag update_tests:groovy --tag ~version_greater_than_equal_to_5x $spec_path
bundle exec rspec -fd $extra_tag_args --tag version_less_than_5x --tag update_tests:groovy --tag ~version_greater_than_equal_to_5x $spec_path
;;
1.*)
setup_es https://download.elastic.co/elasticsearch/elasticsearch/elasticsearch-$ES_VERSION.tar.gz
start_es -Des.script.inline=on -Des.script.indexed=on -Des.script.file=on
# Still have to support ES versions < 2.x so run tests for those.
bundle exec rspec -fd --tag ~secure_integration --tag integration --tag ~version_greater_than_equal_to_5x --tag ~version_greater_than_equal_to_2x $spec_path
bundle exec rspec -fd $extra_tag_args --tag ~version_greater_than_equal_to_5x --tag ~version_greater_than_equal_to_2x $spec_path
;;
*)
download_gradle
build_es master
start_es
bundle exec rspec -fd --tag ~secure_integration --tag integration --tag version_greater_than_equal_to_5x --tag update_tests:painless --tag ~update_tests:groovy --tag ~version_less_than_5x $spec_path
bundle exec rspec -fd $extra_tag_args --tag version_greater_than_equal_to_5x --tag update_tests:painless --tag ~update_tests:groovy --tag ~version_less_than_5x $spec_path
;;
esac
fi
53 changes: 29 additions & 24 deletions lib/logstash/outputs/elasticsearch/http_client.rb
Expand Up @@ -286,32 +286,37 @@ def build_pool(options)
end

def host_to_url(h)
# Build a naked URI class to be wrapped in a SafeURI before returning
# do NOT log this! It could leak passwords
uri_klass = @url_template[:scheme] == 'https' ? URI::HTTPS : URI::HTTP
uri = uri_klass.build(@url_template)

uri.user = h.user || user
uri.password = h.password || password
uri.host = h.host if h.host
uri.port = h.port if h.port
uri.path = h.path if !h.path.nil? && !h.path.empty? && h.path != "/"
uri.query = h.query

# Never override the calculated scheme
raw_scheme = @url_template[:scheme] || 'http'

raw_user = h.user || @url_template[:user]
raw_password = h.password || @url_template[:password]
postfixed_userinfo = raw_user && raw_password ? "#{raw_user}:#{raw_password}@" : nil

raw_host = h.host # Always replace this!
raw_port = h.port || @url_template[:port]

raw_path = !h.path.nil? && !h.path.empty? && h.path != "/" ? h.path : @url_template[:path]
prefixed_raw_path = raw_path && !raw_path.empty? ? raw_path : "/"

parameters = client_settings[:parameters]
if parameters && !parameters.empty?
combined = uri.query ?
Hash[URI::decode_www_form(uri.query)].merge(parameters) :
parameters
query_str = combined.flat_map {|k,v|
values = Array(v)
values.map {|av| "#{k}=#{av}"}
}.join("&")

uri.query = query_str
end
raw_query = if parameters && !parameters.empty?
combined = h.query ?
Hash[URI::decode_www_form(h.query)].merge(parameters) :
parameters
query_str = combined.flat_map {|k,v|
values = Array(v)
values.map {|av| "#{k}=#{av}"}
}.join("&")
query_str
else
h.query
end
prefixed_raw_query = raw_query && !raw_query.empty? ? "?#{raw_query}" : nil

raw_url = "#{raw_scheme}://#{postfixed_userinfo}#{raw_host}:#{raw_port}#{prefixed_raw_path}#{prefixed_raw_query}"

::LogStash::Util::SafeURI.new(uri.normalize)
::LogStash::Util::SafeURI.new(raw_url)
end

def template_exists?(name)
Expand Down
Expand Up @@ -56,7 +56,7 @@ def perform_request(url, method, path, params={}, body=nil)

if url.user
params[:auth] = {
:user => url.user,
:user => CGI.unescape(url.user),
# We have to unescape the password here since manticore won't do it
# for us unless its part of the URL
:password => CGI.unescape(url.password),
Expand Down
8 changes: 3 additions & 5 deletions lib/logstash/outputs/elasticsearch/http_client_builder.rb
Expand Up @@ -140,14 +140,12 @@ def self.setup_ssl(logger, params)

def self.setup_basic_auth(logger, params)
user, password = params["user"], params["password"]
unsafe_password = password && password.value
unsafe_escaped_password = unsafe_password ? CGI.escape(unsafe_password) : nil

return {} unless user && unsafe_escaped_password
return {} unless user && password && password.value

{
:user => user,
:password => unsafe_escaped_password
:user => CGI.escape(user),
:password => CGI.escape(password.value)
}
end

Expand Down
22 changes: 22 additions & 0 deletions spec/fixtures/test_certs/ca/ca.crt
@@ -0,0 +1,22 @@
-----BEGIN CERTIFICATE-----
MIIDmzCCAoOgAwIBAgIUJ3zg8MPyVvlLtNfv27YwTmy8sKAwDQYJKoZIhvcNAQEL
BQAwNDEyMDAGA1UEAxMpRWxhc3RpYyBDZXJ0aWZpY2F0ZSBUb29sIEF1dG9nZW5l
cmF0ZWQgQ0EwHhcNMTcwNzExMjA0ODM3WhcNMjAwNzEwMjA0ODM3WjA0MTIwMAYD
VQQDEylFbGFzdGljIENlcnRpZmljYXRlIFRvb2wgQXV0b2dlbmVyYXRlZCBDQTCC
ASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAIH11HENfB27hsxlPIIZJvBV
gyDRo8T29HEm/mm9hlg4c76xsdXlPoKZ0ENzcEMg+AyKjQxD5cZIACvsd3lbJnca
+8CUcfqKDu1rDq6T5YMX75Gw21ZeJoGy3voCXGvTIoH1aNdRt/8EcWq9P81Mew7A
jh1qhv8WR8Ajxt29OSyWH8VC2WHrihkD/s+3eHCNNpW3CCFtA3uc3sbENVB04A7d
0HzZRD5EfZcZVlcfd5o4ZPuH5PnR4LkZAR2TEgQ8qpruF3m3YMmPvnHaRx5/Bivv
0IuBOAQhkL1w5MzSZVOg3N0u91Krss2tUGLf8c5qnxveqdSKShgVxwI/6P0CaccC
AwEAAaOBpDCBoTAdBgNVHQ4EFgQUba74O4k4WAzgVH9WqCLvypyTj1swbwYDVR0j
BGgwZoAUba74O4k4WAzgVH9WqCLvypyTj1uhOKQ2MDQxMjAwBgNVBAMTKUVsYXN0
aWMgQ2VydGlmaWNhdGUgVG9vbCBBdXRvZ2VuZXJhdGVkIENBghQnfODww/JW+Uu0
1+/btjBObLywoDAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4IBAQAf
jVC71jdUu649ZWketKEqlmAL5K4RmBghKS49Coso1eACngbzVfnh0vfKxF1giIvw
154ZmcAD0bmArO538j+uEvr9kj3ZX8qmvNLRNQ2IcUQaR7N+8Iu8WIqtTIfI07hp
cXHv0jIcq7sFp4BpWDnWGlipFN+ldFRBvaUarrK1QcDAeWyPaUqryY7egecV9PpV
Ebh28RblD5QlA9+sLc5u+zg1vlWg7fLsiGQqNR29dXWmavcamKfo5TOAuzXFlU78
it8DhTCvTnmCIi0NIxJY9rHLHgz/3aDRsBDzCZMQFJ2KfssI6YHZ9LpZ+3iVObr6
jGjKiXeVb9DcAnYR+Ihy
-----END CERTIFICATE-----
27 changes: 27 additions & 0 deletions spec/fixtures/test_certs/ca/ca.key
@@ -0,0 +1,27 @@
-----BEGIN RSA PRIVATE KEY-----
MIIEpAIBAAKCAQEAgfXUcQ18HbuGzGU8ghkm8FWDINGjxPb0cSb+ab2GWDhzvrGx
1eU+gpnQQ3NwQyD4DIqNDEPlxkgAK+x3eVsmdxr7wJRx+ooO7WsOrpPlgxfvkbDb
Vl4mgbLe+gJca9MigfVo11G3/wRxar0/zUx7DsCOHWqG/xZHwCPG3b05LJYfxULZ
YeuKGQP+z7d4cI02lbcIIW0De5zexsQ1UHTgDt3QfNlEPkR9lxlWVx93mjhk+4fk
+dHguRkBHZMSBDyqmu4XebdgyY++cdpHHn8GK+/Qi4E4BCGQvXDkzNJlU6Dc3S73
Uquyza1QYt/xzmqfG96p1IpKGBXHAj/o/QJpxwIDAQABAoIBADvKJGWqpYpsRvTs
Mm1MMwzo2n4T1Lt+PjF8lhmBtzgJKL73s3BLmnmtWBJgHqrTlSr35zJYXnLdly6e
CM1NMSIkyOPtp45zS7DQyx1oL3QjY/VsH0zZ3e9Xopv00B5PMZYGmKhPEU6C9cb8
sEi8QfUkg31nEBp1Xqc4DnrfXllzQiZIVrhHleRcBVtpYVkR3dN5wg+YTwF5n8ru
2Z/N8yuBMrKWqozlUY+x7iDCVXIbp+t3zePfOOhzSdIt4qx372PX6L0HmrI4ZAzw
HkpCQbnlWL55G2CPcnvJF3evbDWoiX8vCYDAQmzFA6X8lAMF9jjKfWlo2Qa1AciN
yqwePKECgYEA0wR9CT51h5PuvGUmKPAfImTNyqQIksibe+AOkU01ldHmBVRfX16f
SKMDKMry4tzXGinVjITonxTdT7jXO5z3754CBoEItdASruikay63DF90hVvP18Zl
OcH/UIBkyvtSjJLEm2/jdfwI+qIOSfhwi5mymXUGVRyEb5i8FORWidcCgYEAnanv
9dk3cx034vG/p36RmI57I1gh6UsEBk53uPZAcecBigDJut2o3vO7/gc/+vROCzk3
Y+EY7tgRDQUh0b9ex+Dq6BkjkZkAKWVCf+u8KDCpW7Xlgm1YDpS2B0EwASgql6Xl
qXu1VgTNc44g0D74yJbs853GWYg5AnV/sRCfgZECgYAklw6nX7E5hSlMea2YQ6ri
Z+BXVwI1kZuEa2GbSGwWQoNEQVEYVGwCSGHv3OEo/Wo/GynwZ8t+ajvF6yNHLvy/
DAMF5bIA9MeIlMaN31fWSWcHCNiNbdV3onAHIXxYxiOWRIza9xfWCZH1A8y+ftnl
Gw2hFm22rG86ep2CceWfmQKBgQCXe8fK90GHoPMpYg066SkK4xr9ApjShfj/9jSh
yjhxN/sKlWc92+t9C8H8eQrIHCNANWE63fQOyBrZ36x20uBGO5x4FG9QXSkCnQAf
2GeYVeji7QnvHxAUMl4S6lctRWJnAkZ/aRT56PNdq5lrfJWcZSaVi2oga/oamrpt
bgNTgQKBgQCGUheUb+51c6hoi7kjuv1uJNyGzirkYA+pNUest2EXVMwMpsLP9hSl
fN11QvMVqRsBnwl2pRnwGilJHl5OO9Z3GBTHsG15EPXhjgUX8r49+GB/HNPP57Ev
lA775+tliHVv6Pl0E3RjhUlSuJIwZZ2ImV+RDryElBKCet4kI/KrIw==
-----END RSA PRIVATE KEY-----
21 changes: 21 additions & 0 deletions spec/fixtures/test_certs/test.crt
@@ -0,0 +1,21 @@
-----BEGIN CERTIFICATE-----
MIIDjDCCAnSgAwIBAgIULhSohxna+aUn+a7UIQmNPLb11p8wDQYJKoZIhvcNAQEL
BQAwNDEyMDAGA1UEAxMpRWxhc3RpYyBDZXJ0aWZpY2F0ZSBUb29sIEF1dG9nZW5l
cmF0ZWQgQ0EwHhcNMTcwNzExMjA0OTEwWhcNMjAwNzEwMjA0OTEwWjAPMQ0wCwYD
VQQDEwR0ZXN0MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAnNOuaiad
YCotO/bSTVJwlR4f1Kdt3gMZeD1i3QKX8QSXACpVeNaTn/6Vflbf/jlWk8i6Mbsv
HeZgdxMe3/CMs8U9fRx2tt4hF5tVivnkRfXbvC5lhYsjg4k2DorkigP/4D4XDonv
pFwB7qKeChnYbe9ADh8DMymKj2Z0LXxSEjuP5VH1O+5jLyddL4gRpCRa5G79vq9m
7rdnq7HTfZ6m3t15AIY4XWZ0VW2epMsflQcbNChcYNMMDhBZzPkhRs7KEEHUV9ye
sGTxGRUBLWNd7rNLY0T0biiUlaWfgbGfUmDKZKofowoo+M9w5rREEnXqrGkc3QmT
zx9XRHqfeEC/ZQIDAQABo4G6MIG3MB0GA1UdDgQWBBS++BZR6DYwjv7jlkS2mVdJ
NtpILDBvBgNVHSMEaDBmgBRtrvg7iThYDOBUf1aoIu/KnJOPW6E4pDYwNDEyMDAG
A1UEAxMpRWxhc3RpYyBDZXJ0aWZpY2F0ZSBUb29sIEF1dG9nZW5lcmF0ZWQgQ0GC
FCd84PDD8lb5S7TX79u2ME5svLCgMBoGA1UdEQQTMBGCCWxvY2FsaG9zdIcEfwAA
ATAJBgNVHRMEAjAAMA0GCSqGSIb3DQEBCwUAA4IBAQBugjYG5LK91A1awDYTsrrT
Bmi8TiOeHa69Qia4SrFpwua648TpC5esaIC4uAJlTVP5MwcMB/5+lZOcUjVgFIss
Z4mdRgExXMAoDiI5OOqXcCiGYWv/jGJzKj4EZ65UZADVrGDFmZPW99g4XDInc27W
lmZETkjriueku7gH4eEOdfSX9jv6M+mYhRJlllWlzk77zzENtFn4uCQmd9bzFDQM
hOxi+tif9SrGEAoJoSP204vunLIDNve2KycVXuQkhZO+Q5JYmN38DqFrjC/U5JbC
IIdTES3+iydEHevB3biI251NrQkg1eOOTG376viFZVmZB9e75waf8JLKKswNmubU
-----END CERTIFICATE-----
27 changes: 27 additions & 0 deletions spec/fixtures/test_certs/test.key
@@ -0,0 +1,27 @@
-----BEGIN RSA PRIVATE KEY-----
MIIEowIBAAKCAQEAnNOuaiadYCotO/bSTVJwlR4f1Kdt3gMZeD1i3QKX8QSXACpV
eNaTn/6Vflbf/jlWk8i6MbsvHeZgdxMe3/CMs8U9fRx2tt4hF5tVivnkRfXbvC5l
hYsjg4k2DorkigP/4D4XDonvpFwB7qKeChnYbe9ADh8DMymKj2Z0LXxSEjuP5VH1
O+5jLyddL4gRpCRa5G79vq9m7rdnq7HTfZ6m3t15AIY4XWZ0VW2epMsflQcbNChc
YNMMDhBZzPkhRs7KEEHUV9yesGTxGRUBLWNd7rNLY0T0biiUlaWfgbGfUmDKZKof
owoo+M9w5rREEnXqrGkc3QmTzx9XRHqfeEC/ZQIDAQABAoIBAAm4XREbP5ncQ116
GOLN/0hey55EmlyuWH/JXj1QkdZQcIOEHDQXKKM8BkwEWnHJYAJc6J14ep0h0EzR
FJLQuAfUa9E7WGhRMD/kUtMAVhO3/1yUi5pRW2wlrwILvcqIIO3nK0qtZfsL8Nq3
nZAGthFqSNAXP/2Fz56/vOes0vFqQpHC9y+xzQVm9Kr/bZeB0gWH1Eq32Dz2Q8pW
E1BNgy48az20AcH8KiJCy5JjByDkcBjBPm8S074Hab9VPvLEnKrpApNZT3V/cLVd
t0UmGT937g1DuqwYBl6eLB+FgOZhNjONtF7llHDYWtx0r9XaKplGtmMeW1rDRLp4
XkDeKqkCgYEA54xm+Uc1SLGgnzPQb88Sk+H6ffFy4fkzS2FTeEm7NlKpFLtPVNx+
wZys0GobYn7XFq0lGODWhuFJbN8vm8aGZ2xgKv3213yKWOjn/UBP8D8JyC3HQs5f
atf6a15z7bp+Vrab+tn0dOi4Jz6FhNxmhwFVS3n+e7fZo7gUxAAOxTcCgYEArWNI
c2f+Qf44lskqrN2NNNY44/lYEDB5WWKT8q+9Z/4MuFXmYQvQHrdvI1yuYfXSLUqN
VeH4j7V25nQXBbB1zCvHmjP2/t00EwhRh1Z9iNA74eukRJrzvMvgugSioVc3PpDx
yb3+7kT68iE3LHWWW0If0ENb3CX6/OyAiJRG7kMCgYEA0//0+B9+ZcRcb+cc3IIX
XFb25gD/Um67zDScG/JF+oLMVDL7e5M2a0Zr45aC5DeF7zkwUgrp4Cy88XWXPWUT
AfZ0RmiobLuWX7k/TtxnVGwjJKjlXAFf049TtKKSOgMaUYJ4ZcDQ1YmNskDINtEk
/k72LVjQ6611EzUjriDvZRkCgYAmtAPHJw59Yqb1GaB6B9ZuVedLFCyRKJDd4ABQ
auQno3DpcNtFDGL/iEi5pwWR/lJVI9AavJ9ETOhmlsFQ1svksF0U0cavq2blXLT+
NdM9x+WmD3iSi9gea5AVVdWLmDFPuQEP3GZcf29YvwtW1ESkyETbsz19DclRzeT/
F8IhiwKBgCrTlgDzZrvMKzD2VHNPLkr2ixJwEolTQFwhklAFeFvlUkI0XwBHkfwS
hQ+EQosZaPIQ5pupn8UFmpfGpv7f4vyTSVtbkay7eBIgFY8jPZFkM1zqtgwRerN1
5HNArAuzAH9LiauuZrAhFolwPfEdSWxvJhvfPE/aM5OpbQRUyQb4
-----END RSA PRIVATE KEY-----

0 comments on commit e1b120d

Please sign in to comment.