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

kops compares sha1 with sha256 while checking hash of nodeup #8242

Closed
yzx777 opened this issue Jan 1, 2020 · 14 comments · Fixed by #8243
Closed

kops compares sha1 with sha256 while checking hash of nodeup #8242

yzx777 opened this issue Jan 1, 2020 · 14 comments · Fixed by #8243

Comments

@yzx777
Copy link

yzx777 commented Jan 1, 2020

1. What kops version are you running?

Version 1.15.0

2. What Kubernetes version are you running?

v1.15.7

3. What cloud provider are you using?

AWS

4. What commands did you run? What is the simplest way to reproduce this issue?

I created my cluster with --kubernetes-version according to the doc How to use kops in AWS China Region.
Note that I only downloaded the "$url.sha1" file, not the sha256 file.

5. What happened after the commands executed?

kops generated user-data to initialize the master nodes, but compared sha1 with sha256 while checking hash of the file "nodeup". So the initialization couldn't be done.

6. The nodes journalctl logs

== nodeup corrupted, hash 9604ef18267ad7b5cf4cebbf7ab64423cf5bb0342d169c608ac6376e6af26d81 doesn't match expected f22d740fdbe2fc92e9b359bf23c5b6d7caa0aba9 ==
== Hash validation of https://s3.cn-north-1.amazonaws.com.cn/my_bucket/kops/1.15.0/linux/amd64/nodeup failed. Retrying. ==
All downloads failed; sleeping before retrying

7. Anything else do we need to know?

I notice that the method findHash in pkg/assets/builder.go will try ".sha256" first, then ".sha1".
It means that we use sha1 when couldn't get sha256.

for _, ext := range []string{".sha256", ".sha1"} {
    hashURL := u.String() + ext
    b, err := vfs.Context.ReadFile(hashURL, vfs.WithBackoff(backoff))
    if err != nil {
        // Try to log without being too alarming - issue #7550
        if ext == ".sha256" {
            klog.V(2).Infof("unable to read new sha256 hash file (is this an older/unsupported kubernetes release?) %q: %v", hashURL, err)
        } else {
            klog.V(2).Infof("unable to read hash file %q: %v", hashURL, err)
        }
        continue
    }
    hashString := strings.TrimSpace(string(b))
    klog.V(2).Infof("Found hash %q for %q", hashString, u)

    // Accept a hash string that is `<hash> <filename>`
    fields := strings.Fields(hashString)
    if len(fields) == 0 {
        klog.Infof("hash file was empty %q", hashURL)
        continue
    }
    return hashing.FromString(fields[0])
}

But the NodeUpTemplate in pkg/model/resources/nodeup.go looks like below.
It just uses sha256sum.

validate-hash() {
  local -r file="$1"
  local -r expected="$2"
  local actual

  actual=$(sha256sum ${file} | awk '{ print $1 }') || true
  if [[ "${actual}" != "${expected}" ]]; then
    echo "== ${file} corrupted, hash ${actual} doesn't match expected ${expected} =="
    return 1
  fi
}

So, would it be better to do like this in NodeUpTemplate? (length of sha1 is 40.)

validate-hash() {
  local -r file="$1"
  local -r expected="$2"
  local actual
  local hash_cmd=sha256sum

  if [[ ${#expected} == 40 ]]; then
    hash_cmd=sha1sum
  fi

  actual=$(${hash_cmd} ${file} | awk '{ print $1 }') || true
  if [[ "${actual}" != "${expected}" ]]; then
    echo "== ${file} corrupted, hash ${actual} doesn't match expected ${expected} =="
    return 1
  fi
}
@johngmyers
Copy link
Member

I believe it would be better to fix the documentation to specify downloading the sha256

@johngmyers
Copy link
Member

johngmyers commented Jan 1, 2020

Looks like the sha256 started being provided around 1.14.7.

@johngmyers
Copy link
Member

Since kops 1.15, kops assets are only verified with the sha256 per this commit.

@johngmyers
Copy link
Member

@yzx777 I'd appreciate it if you could test the modified script in #8243

@yzx777
Copy link
Author

yzx777 commented Jan 2, 2020

@johngmyers
I have already run my cluster to do like that. It's OK to download the sha256 file.
But I think it's not the root of this problem.It still uses sha1 when can't get sha256 but sha1 for some reason(may be network problem). Or can I open a PR?

@johngmyers
Copy link
Member

It still uses sha1 when can't get sha256 but sha1 for some reason(may be network problem).

I'm sorry, I don't understand this sentence. Could you rephrase?

Kops only uses sha256 for kops assets and this is both intended and desirable.

@yzx777
Copy link
Author

yzx777 commented Jan 2, 2020

Just like I said above, the method findHash in pkg/assets/builder.go will try ".sha256" first, then ".sha1". The var NODEUP_HASH in NodeUpTemplate would be sha1, when kops can't get sha256 file, but can get sha1 file.

@johngmyers
Copy link
Member

It would be much more likely for it to be unable to get nodeup itself than its sha256 hash. This is not worth reducing the security of the check.

@yzx777
Copy link
Author

yzx777 commented Jan 4, 2020

@justinsb
Any suggestions?

@elblivion
Copy link
Contributor

@yzx777 we had a similar issue just now on upgrading, because we manage Kops assets in our own S3 buckets for region independence (see #6368) - TL;DR anyone who is copying the Kops assets (nodeup, etc) and wasn't copying the equivalent sha256 hash file will have this issue.

For us it was fixed by:

  • Ensuring the .sha256 hash files are uploaded so they can be found under KOPS_BASE_URL
  • Run your kops update or kops create in whatever your workflow is
  • Check that NODEUP_HASH in the instance user-data scripts is the sha256 checksum of the nodeup file.

@yzx777
Copy link
Author

yzx777 commented Jan 8, 2020

@elblivion I still think there is a problem that kops does wrong action according to the external environment. It's illogical.

@elblivion
Copy link
Contributor

@yzx777 agreed, I think the problem is that .sha1 is still being taken into account here:

kops/pkg/assets/builder.go

Lines 316 to 339 in b6be949

for _, ext := range []string{".sha256", ".sha1"} {
hashURL := u.String() + ext
b, err := vfs.Context.ReadFile(hashURL, vfs.WithBackoff(backoff))
if err != nil {
// Try to log without being too alarming - issue #7550
if ext == ".sha256" {
klog.V(2).Infof("unable to read new sha256 hash file (is this an older/unsupported kubernetes release?) %q: %v", hashURL, err)
} else {
klog.V(2).Infof("unable to read hash file %q: %v", hashURL, err)
}
continue
}
hashString := strings.TrimSpace(string(b))
klog.V(2).Infof("Found hash %q for %q", hashString, u)
// Accept a hash string that is `<hash> <filename>`
fields := strings.Fields(hashString)
if len(fields) == 0 {
klog.Infof("hash file was empty %q", hashURL)
continue
}
return hashing.FromString(fields[0])
}
}

@yzx777
Copy link
Author

yzx777 commented Jan 8, 2020

@elblivion It seems to do this to support previous versions of kubernetes.

@johngmyers
Copy link
Member

Kubernetes versions before 1.15.4 or thereabouts don't have sha256 files. Since kops supports those earlier versions, it needs to be able to verify sha1 files for Kubernetes assets. When kops drops support for Kubernetes versions earlier from that, it will likely drop support for verifying sha1 files altogether.

Kops does not, however, need to support kops assets from earlier versions. So it does not support verifying anything other than sha256 files for kops assets. The kops assets have sha256 files. If someone is copying the assets locally, they need to also copy the sha256 files for the kops ones.

Now that the aws-china.md script has been fixed, I don't see a problem.

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 a pull request may close this issue.

3 participants